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

Use kubernetes queue in kubernetes hybrid executors #23048

Merged
merged 7 commits into from
May 5, 2022

Conversation

tanelk
Copy link
Contributor

@tanelk tanelk commented Apr 16, 2022

When using "hybrid" executors (CeleryKubernetesExecutor or LocalKubernetesExecutor), then the clear_not_launched_queued_tasks mechnism in the KubernetesExecutor can reset the queued tasks, that were given to the other executor.

KuberneterExecutor should limit itself to the configured queue when working in the "hybrid" mode.

This is reported at least once: #21225 (comment) , but has not been dealt with.
Fixes #22554


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
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.

@boring-cyborg boring-cyborg bot added provider:cncf-kubernetes Kubernetes provider related issues area:Scheduler including HA (high availability) scheduler labels Apr 16, 2022
@tanelk tanelk closed this Apr 18, 2022
@tanelk tanelk reopened this Apr 18, 2022
@vandanthaker vandanthaker mentioned this pull request Apr 18, 2022
2 tasks
@tanelk
Copy link
Contributor Author

tanelk commented Apr 22, 2022

Pinging @ashb for a review of this bugfix.

@tanelk
Copy link
Contributor Author

tanelk commented Apr 25, 2022

@uranusjr, could you take a look at this bugfix

@jensenity
Copy link

any update on this guys?

@balosh-daniel
Copy link

this can help us thanks, eagerly waiting for it 👍

@tanelk
Copy link
Contributor Author

tanelk commented May 4, 2022

Pinging @ashb and @uranusjr again - hopefully you have time to take a look at this now that 2.3 is out.

Copy link
Member

@ashb ashb left a comment

Choose a reason for hiding this comment

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

Looks good, and will be included in 2.3.1

@ashb ashb added this to the Airflow 2.3.1 milestone May 4, 2022
@ashb
Copy link
Member

ashb commented May 4, 2022

@tanelk Could you rebase please?

@github-actions github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label May 4, 2022
@github-actions
Copy link

github-actions bot commented May 4, 2022

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

@ashb ashb merged commit ae19eab into apache:main May 5, 2022
@snjypl
Copy link
Contributor

snjypl commented May 5, 2022

@tanelk i was thinking instead of filtering by TaskInstance.queue should we be filtering by TaskInstance.queued_by_job_id?

that way the clear_not_launched_queued_tasks will only handle the TaskInstance created by the particular Job.

there is a similar issue #23356 with backfill job.

@tanelk
Copy link
Contributor Author

tanelk commented May 6, 2022

@snjypl not "instead of", but more likely "additionally". The queue filter is needed for this particular bug because every job can queue celery and kubernetes tasks.

@tanelk tanelk deleted the kubernetes_executor_queue branch May 6, 2022 04:02
@ephraimbuddy ephraimbuddy added the type:bug-fix Changelog: Bug Fixes label May 8, 2022
ephraimbuddy pushed a commit that referenced this pull request May 8, 2022
When using "hybrid" executors (`CeleryKubernetesExecutor` or `LocalKubernetesExecutor`),
then the `clear_not_launched_queued_tasks` mechnism in the `KubernetesExecutor` can
reset the queued tasks, that were given to the other executor.

`KuberneterExecutor` should limit itself to the configured queue when working in the
"hybrid" mode.

(cherry picked from commit ae19eab)
ephraimbuddy pushed a commit that referenced this pull request May 21, 2022
When using "hybrid" executors (`CeleryKubernetesExecutor` or `LocalKubernetesExecutor`),
then the `clear_not_launched_queued_tasks` mechnism in the `KubernetesExecutor` can
reset the queued tasks, that were given to the other executor.

`KuberneterExecutor` should limit itself to the configured queue when working in the
"hybrid" mode.

(cherry picked from commit ae19eab)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:Scheduler including HA (high availability) scheduler full tests needed We need to run full set of tests for this PR to merge provider:cncf-kubernetes Kubernetes provider related issues type:bug-fix Changelog: Bug Fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants