Skip to content

Separate Kubernetes pod_launcher from core airflow#15165

Merged
dimberman merged 10 commits into
apache:masterfrom
astronomer:refactor-k8s-operator
Apr 5, 2021
Merged

Separate Kubernetes pod_launcher from core airflow#15165
dimberman merged 10 commits into
apache:masterfrom
astronomer:refactor-k8s-operator

Conversation

@dimberman

Copy link
Copy Markdown
Contributor

Currently, the KubernetesPodOperator uses the pod_launcher class in airflow core. This means that if we need to fix a bug in the KubernetesPodOperator such as #15137 then the new cncf.kubernetes package will require an Airflow upgrade. Since we hope to release providers in a much faster cadence than Airflow core releases, we should separate this dependency.


^ 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 (k8s) provider related issues area:providers area:Scheduler including HA (high availability) scheduler labels Apr 2, 2021
@dimberman

Copy link
Copy Markdown
Contributor Author

cc: @SamWheating

Comment thread airflow/kubernetes/pod_launcher.py Outdated
Comment on lines 26 to 28

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we note in UPDATING.md that this class is deprecated?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@eladkal added

@github-actions

github-actions Bot commented Apr 2, 2021

Copy link
Copy Markdown
Contributor

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

@dimberman dimberman force-pushed the refactor-k8s-operator branch 2 times, most recently from 8ddcf6c to 01b2f75 Compare April 3, 2021 04:28
Comment thread airflow/kubernetes/pod_launcher.py Outdated
Comment on lines 246 to 262

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is all the executor used the pod launcher for?!

I was expecting this to be a much bigger change

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@ashb It makes sense. The KubernetesExecutor is fire and forget. We don't monitor the task via the pod launcher for the KubernetesExecutor, just monitor the task state via the job watcher.

Comment thread airflow/kubernetes/pod_launcher.py Outdated
Comment thread airflow/kubernetes/pod_launcher.py Outdated
@dimberman dimberman force-pushed the refactor-k8s-operator branch from 166c529 to c91b2e2 Compare April 5, 2021 15:18
kaxil
kaxil previously requested changes Apr 5, 2021
Comment thread airflow/kubernetes/pod_launcher.py Outdated
@dimberman dimberman force-pushed the refactor-k8s-operator branch from de374b3 to 883c55f Compare April 5, 2021 17:03
@kaxil kaxil self-requested a review April 5, 2021 17:19
@kaxil kaxil dismissed their stale review April 5, 2021 17:20

Fixed

@potiuk potiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

One more NIT

@potiuk potiuk Apr 5, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should we also add the deprecation warning directly here (or even move the warning from the old pod_launcher)? This way even if someone by mistake imports PodLauncher/PodStatus directly from there (because IDE will provide this as first-class option), will still get the warning.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@potiuk I added the deprecation to the deprecated class. good catch!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yep.. Waiting for push to approve it ;)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@potiuk oh oops 🤦 . Pushed!

Currently, the KubernetesPodOperator uses the pod_launcher class in airflow core. This means that if we need to fix a bug in the KubernetesPodOperator such as apache#15137 then the new cncf.kubernetes package will require an Airflow upgrade. Since we hope to release providers in a much faster cadence than Airflow core releases, we should separate this dependency.
@dimberman dimberman force-pushed the refactor-k8s-operator branch from 82e88b9 to 5d0b0ca Compare April 5, 2021 20:29
@github-actions github-actions Bot added the okay to merge It's ok to merge this PR as it does not require more tests label Apr 5, 2021
@github-actions

github-actions Bot commented Apr 5, 2021

Copy link
Copy Markdown
Contributor

The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest master or amend the last commit of the PR, and push it with --force-with-lease.

@dimberman dimberman merged commit 6d7a70b into apache:master Apr 5, 2021
@dimberman dimberman deleted the refactor-k8s-operator branch April 5, 2021 21:45
dimberman added a commit to astronomer/airflow that referenced this pull request Apr 6, 2021
apache#15165 introduced a bug in the
kubernetes_executor tests. This fixes that bug by changing pytest mock
potiuk pushed a commit that referenced this pull request Apr 6, 2021
#15165 introduced a bug in the
kubernetes_executor tests. This fixes that bug by changing pytest mock
leahecole pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Sep 17, 2021
apache/airflow#15165 introduced a bug in the
kubernetes_executor tests. This fixes that bug by changing pytest mock

GitOrigin-RevId: e1beefc1123ba340a2565c743a0723d135bfc3a3
leahecole pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Sep 23, 2021
apache/airflow#15165 introduced a bug in the
kubernetes_executor tests. This fixes that bug by changing pytest mock

GitOrigin-RevId: e1beefc1123ba340a2565c743a0723d135bfc3a3
leahecole pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Nov 27, 2021
apache/airflow#15165 introduced a bug in the
kubernetes_executor tests. This fixes that bug by changing pytest mock

GitOrigin-RevId: e1beefc1123ba340a2565c743a0723d135bfc3a3
leahecole pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Mar 10, 2022
apache/airflow#15165 introduced a bug in the
kubernetes_executor tests. This fixes that bug by changing pytest mock

GitOrigin-RevId: e1beefc1123ba340a2565c743a0723d135bfc3a3
leahecole pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Jun 4, 2022
apache/airflow#15165 introduced a bug in the
kubernetes_executor tests. This fixes that bug by changing pytest mock

GitOrigin-RevId: e1beefc1123ba340a2565c743a0723d135bfc3a3
kosteev pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Jul 9, 2022
apache/airflow#15165 introduced a bug in the
kubernetes_executor tests. This fixes that bug by changing pytest mock

GitOrigin-RevId: e1beefc1123ba340a2565c743a0723d135bfc3a3
leahecole pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Aug 27, 2022
apache/airflow#15165 introduced a bug in the
kubernetes_executor tests. This fixes that bug by changing pytest mock

GitOrigin-RevId: e1beefc1123ba340a2565c743a0723d135bfc3a3
leahecole pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Oct 4, 2022
apache/airflow#15165 introduced a bug in the
kubernetes_executor tests. This fixes that bug by changing pytest mock

GitOrigin-RevId: e1beefc1123ba340a2565c743a0723d135bfc3a3
aglipska pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Oct 7, 2022
apache/airflow#15165 introduced a bug in the
kubernetes_executor tests. This fixes that bug by changing pytest mock

GitOrigin-RevId: e1beefc1123ba340a2565c743a0723d135bfc3a3
leahecole pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Dec 7, 2022
apache/airflow#15165 introduced a bug in the
kubernetes_executor tests. This fixes that bug by changing pytest mock

GitOrigin-RevId: e1beefc1123ba340a2565c743a0723d135bfc3a3
leahecole pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Jan 27, 2023
apache/airflow#15165 introduced a bug in the
kubernetes_executor tests. This fixes that bug by changing pytest mock

GitOrigin-RevId: e1beefc1123ba340a2565c743a0723d135bfc3a3
kosteev pushed a commit to kosteev/composer-airflow-test-copybara that referenced this pull request Sep 12, 2024
apache/airflow#15165 introduced a bug in the
kubernetes_executor tests. This fixes that bug by changing pytest mock

GitOrigin-RevId: e1beefc1123ba340a2565c743a0723d135bfc3a3
kosteev pushed a commit to kosteev/composer-airflow-test-copybara that referenced this pull request Sep 12, 2024
apache/airflow#15165 introduced a bug in the
kubernetes_executor tests. This fixes that bug by changing pytest mock

GitOrigin-RevId: e1beefc1123ba340a2565c743a0723d135bfc3a3
kosteev pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Sep 17, 2024
apache/airflow#15165 introduced a bug in the
kubernetes_executor tests. This fixes that bug by changing pytest mock

GitOrigin-RevId: e1beefc1123ba340a2565c743a0723d135bfc3a3
kosteev pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Nov 7, 2024
apache/airflow#15165 introduced a bug in the
kubernetes_executor tests. This fixes that bug by changing pytest mock

GitOrigin-RevId: e1beefc1123ba340a2565c743a0723d135bfc3a3
kosteev pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request May 1, 2025
apache/airflow#15165 introduced a bug in the
kubernetes_executor tests. This fixes that bug by changing pytest mock

GitOrigin-RevId: e1beefc1123ba340a2565c743a0723d135bfc3a3
kosteev pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request May 22, 2025
apache/airflow#15165 introduced a bug in the
kubernetes_executor tests. This fixes that bug by changing pytest mock

GitOrigin-RevId: e1beefc1123ba340a2565c743a0723d135bfc3a3
kosteev pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Sep 17, 2025
apache/airflow#15165 introduced a bug in the
kubernetes_executor tests. This fixes that bug by changing pytest mock

GitOrigin-RevId: e1beefc1123ba340a2565c743a0723d135bfc3a3
kosteev pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Oct 15, 2025
apache/airflow#15165 introduced a bug in the
kubernetes_executor tests. This fixes that bug by changing pytest mock

GitOrigin-RevId: e1beefc1123ba340a2565c743a0723d135bfc3a3
kosteev pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Feb 20, 2026
apache/airflow#15165 introduced a bug in the
kubernetes_executor tests. This fixes that bug by changing pytest mock

GitOrigin-RevId: e1beefc1123ba340a2565c743a0723d135bfc3a3
kosteev pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Apr 24, 2026
apache/airflow#15165 introduced a bug in the
kubernetes_executor tests. This fixes that bug by changing pytest mock

GitOrigin-RevId: e1beefc1123ba340a2565c743a0723d135bfc3a3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:providers area:Scheduler including HA (high availability) scheduler okay to merge It's ok to merge this PR as it does not require more tests provider:cncf-kubernetes Kubernetes (k8s) provider related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants