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

More meaningful name for @task.kubernetes pods #46462

Closed
wants to merge 25 commits into from

Conversation

insomnes
Copy link
Contributor

@insomnes insomnes commented Feb 5, 2025

Use name based on python callable in @task.kubernetes pod name generation

  • generate specific pod name base by provided python callable
  • drop uuid usage with no name (random is controlled by random_name_suffix arg, there is no need for extra UUID usage)
  • add specific pod naming tests for decorator flow because decorator distinguishes between name=None and no name argument provided

closes: #46464


^ 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.

Copy link
Contributor

@RNHTTR RNHTTR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What will happen if two Dag runs are triggered at the same time, and two TaskInstances are created from the same task simultaneously?

@insomnes
Copy link
Contributor Author

insomnes commented Feb 5, 2025

What will happen if two Dag runs are triggered at the same time, and two TaskInstances are created from the same task simultaneously?

@RNHTTR
If random_name_suffix arg is set (as default) they would have two different names by separate random suffixes which are generated as part of pod request object during execute calls in KPO by build_pod_request_obj method.

If user has disabled name randomization by random_name_suffix=False -- that's more on user side to ensure name uniqueness.

I am happy to add a test for this case if you can guide me a bit on how to setup such test.

@insomnes insomnes force-pushed the task-k8s-callable-name branch from 0fcc0cc to bf8ea71 Compare February 5, 2025 23:00
@RNHTTR
Copy link
Contributor

RNHTTR commented Feb 6, 2025

Looks good imo, but tests are failing now

@insomnes insomnes force-pushed the task-k8s-callable-name branch from bf8ea71 to 75b03e8 Compare February 6, 2025 08:02
@insomnes
Copy link
Contributor Author

insomnes commented Feb 6, 2025

I think: the problem is on the CI side with k8s cluster tests and not in the code, will wait a bit and retry by rebasing the branch. My last attempt failed to set the environment, and previous failures were about failing jobs.

…ration

- generate specific pod name base by provided python callable
- drop uuid usage by default (random is controlled by `random_name_suffix` arg)
- add specific pod naming tests for decorator flow
@insomnes insomnes force-pushed the task-k8s-callable-name branch from 75b03e8 to 45f68e4 Compare February 6, 2025 11:11
@insomnes
Copy link
Contributor Author

insomnes commented Feb 6, 2025

Oh, now there is conflict after the provider move to a new structure. Will check it a bit later today

@potiuk
Copy link
Member

potiuk commented Feb 6, 2025

Yeah... Sorry :) . It was finaly green, so I merged it. But I think in this case git will properly fix stuff when you rebase - automatically

0BVer and others added 11 commits February 6, 2025 21:02
…che#45864)

* Allow passing empty labels in the driver config

* Fix formatting

* Fix formatting

---------

Co-authored-by: Maxim Logvinenko <[email protected]>
…pache#46219)

* BREAKING CHANGE: replace Airflow config by conx extras in SMTP provider

* fix static checks
We can't just check if CeleryExecutor is one of the executors when
determining if we can expose the automount param, we need to make sure
it's the _only_ executor - the others need it.
* Add XCom update API

* Add tests for XCom update API

* Fix failures on execution
apache#46480)

Closes apache#46477

The chart version 1.16.0 would be released before airflow v3, so it's better to keep api server in beta so it's easier to make changes.

Signed-off-by: Andrii Korotkov <[email protected]>
feluelle and others added 11 commits February 6, 2025 21:02
* Fix task sdk client dry-run mode

The `run_after` field was missing in the fake dag run response.

* Fix trailing whitespaces
* refactor: Moved microsoft winrm provider

* refactor: fixed static checks
* Update console link to filter on multiple instance ids
* Add state filter to task page.

* Refactor task instance and dagrun states to constants util.
* Move CNCF Kubernetes to new provider structure

* Fix doc include path and k8s test

* Fix taskflow tutorial

* Fix test_project_structure

* Strip src. prefix instead of replacing all src.

Co-authored-by: Kalyan R <[email protected]>

* Merge fix get_classes_from_file apache#46454

* Fix TestCncfProviderProjectStructure
- rename PROVIDER from "cncf" to "cncf/kubernetes"
- remove MISSING_EXAMPLES_FOR_CLASSES

* Fix k8s CI requirements

* fixup! Fix k8s CI requirements

---------

Co-authored-by: Kalyan R <[email protected]>
Co-authored-by: Jarek Potiuk <[email protected]>
The big change here (other than just moving code around) is to introduce a
conceptual separation between Definition/Execution time and Scheduler time.

This means that the expansion of tasks (creating the TaskInstance rows with
different map_index values) is now done on the scheduler, and we now
deserialize to different classes. For example, when we deserialize the
`DictOfListsExpandInput` it gets turned into an instance of
SchedulerDictOfListsExpandInput. This is primarily designed so that DB access
is kept 100% out of the TaskSDK.

Some of the changes here are on the "wat" side of the scale, and this is
mostly designed to not break 100% of our tests, and we have apache#45549 to look at
that more holistically.

To support the "reduce" style task which takes as input a sequence of all the
pushed (mapped) XCom values, and to keep the previous behaviour of not loading
all values in to memory at once, we have added a new HEAD route to the Task
Execution interface that returns the number of mapped XCom values so that it
is possible to implement `__len__` on the new LazyXComSequence class.

I have deleted a tranche of tests from tests/models that were to do with
runtime behavoir and and now tested in the TaskSDK instead.
…he#46485)

* Fix bug in query invalidation and remove custom predicate logic

* Address PR feedback

* Only use key fn when we have props to pass
* Moving yandex provider to new provider structure

* fixup! Moving yandex provider to new provider structure

---------

Co-authored-by: Jarek Potiuk <[email protected]>
@potiuk
Copy link
Member

potiuk commented Feb 6, 2025

still conflicts to solve :(

@insomnes
Copy link
Contributor Author

insomnes commented Feb 6, 2025

Holy moly I am so sorry for this mess, and I am really sorry for the ping for the whole repo I will close this one and apply my changes to the new structure, that would be easier. I am sorry for bothering you all

@insomnes insomnes closed this Feb 6, 2025
@insomnes insomnes deleted the task-k8s-callable-name branch February 6, 2025 20:24
@potiuk
Copy link
Member

potiuk commented Feb 6, 2025

No worries :)

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.

More meaningful name for @task.kubernetes pods by default