Skip to content

Add multi-team support for KubernetesExecutor#61798

Merged
jscheffl merged 6 commits into
apache:mainfrom
vbottu:aip67-kubernetes-executor-multi-team
Mar 7, 2026
Merged

Add multi-team support for KubernetesExecutor#61798
jscheffl merged 6 commits into
apache:mainfrom
vbottu:aip67-kubernetes-executor-multi-team

Conversation

@vbottu
Copy link
Copy Markdown
Contributor

@vbottu vbottu commented Feb 12, 2026


Was generative AI tooling used to co-author this PR?
  • Yes (please specify the tool below)

  • Read the Pull Request Guidelines for more information. Note: commit author/co-author name and email in commits become permanently public when merged.
  • For fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
  • When adding dependency, check compliance with the ASF 3rd Party License Policy.
  • For significant user-facing changes create newsfragment: {pr_number}.significant.rst or {issue_number}.significant.rst, in airflow-core/newsfragments.

@boring-cyborg boring-cyborg Bot added area:providers provider:cncf-kubernetes Kubernetes (k8s) provider related issues labels Feb 12, 2026
@vbottu vbottu force-pushed the aip67-kubernetes-executor-multi-team branch 2 times, most recently from 06ecff2 to 39656ac Compare February 12, 2026 07:12
@vbottu
Copy link
Copy Markdown
Contributor Author

vbottu commented Feb 12, 2026

Note on # type: ignore[assignment] for kube_image in kube_config.py:

Switching config reads from conf.get() (AirflowConfigParser) to self._conf.get() (ExecutorConf) changes the return type mypy sees — AirflowConfigParser.get() has overloads that resolve to str, while ExecutorConf.get() returns str | None. This causes mypy to infer worker_container_repository and worker_container_tag as str | None, which then makes the self.kube_image = f"..." branch infer as str while the else: self.kube_image = None branch is rejected.

The type: ignore preserves the exact same runtime and mypy behavior as the original code. The underlying type inconsistency is pre-existing: kube_image can be None at runtime, but PodGenerator.construct_pod() declares kube_image: str. Fixing this properly would require changing the construct_pod signature (out of scope for this PR). Notably, passing None here is intentional — the Kubernetes client omits None fields during serialization, whereas an empty string would produce 'image': '' in the pod spec.

Copy link
Copy Markdown
Contributor

@jscheffl jscheffl left a comment

Choose a reason for hiding this comment

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

Some small revision needed, else from code look good in my view

@jscheffl
Copy link
Copy Markdown
Contributor

witching config reads from conf.get() (AirflowConfigParser) to self._conf.get() (ExecutorConf) changes the return type mypy sees — AirflowConfigParser.get() has overloads that resolve to str, while ExecutorConf.get() returns str | None. This causes mypy to infer worker_container_repository and worker_container_tag as str | None, which then makes the self.kube_image = f"..." branch infer as str while the else: self.kube_image = None branch is rejected.

The type: ignore preserves the exact same runtime and mypy behavior as the original code. The underlying type inconsistency is pre-existing: kube_image can be None at runtime, but PodGenerator.construct_pod() declares kube_image: str. Fixing this properly would require changing the construct_pod signature (out of scope for this PR). Notably, passing None here is intentional — the Kubernetes client omits None fields during serialization, whereas an empty string would produce 'image': '' in the pod spec.

Can you maybe add an inline comment about this for people seeing this after merging and looking at the code?

@vbottu vbottu force-pushed the aip67-kubernetes-executor-multi-team branch from 39656ac to ddecc3f Compare February 14, 2026 20:19
@vbottu
Copy link
Copy Markdown
Contributor Author

vbottu commented Feb 14, 2026

witching config reads from conf.get() (AirflowConfigParser) to self._conf.get() (ExecutorConf) changes the return type mypy sees — AirflowConfigParser.get() has overloads that resolve to str, while ExecutorConf.get() returns str | None. This causes mypy to infer worker_container_repository and worker_container_tag as str | None, which then makes the self.kube_image = f"..." branch infer as str while the else: self.kube_image = None branch is rejected.
The type: ignore preserves the exact same runtime and mypy behavior as the original code. The underlying type inconsistency is pre-existing: kube_image can be None at runtime, but PodGenerator.construct_pod() declares kube_image: str. Fixing this properly would require changing the construct_pod signature (out of scope for this PR). Notably, passing None here is intentional — the Kubernetes client omits None fields during serialization, whereas an empty string would produce 'image': '' in the pod spec.

Can you maybe add an inline comment about this for people seeing this after merging and looking at the code?

Added inline comments

@vbottu
Copy link
Copy Markdown
Contributor Author

vbottu commented Feb 18, 2026

#60912 @o-nikolas Please review this PR

Copy link
Copy Markdown
Contributor

@o-nikolas o-nikolas left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! A few comments. Also what kind of manual QA have you done?

@o-nikolas o-nikolas added the full tests needed We need to run full set of tests for this PR to merge label Feb 18, 2026
@o-nikolas
Copy link
Copy Markdown
Contributor

Hey @vbottu just checking in to see how this is coming along?

@o-nikolas
Copy link
Copy Markdown
Contributor

Hey @vbottu Checking in again on this one. Any progress on the testing or feedback from the last review?

@vbottu
Copy link
Copy Markdown
Contributor Author

vbottu commented Feb 27, 2026

Hello @o-nikolas , couldn't get to it, I will look into it tomorrow if that's okay.

@o-nikolas
Copy link
Copy Markdown
Contributor

Hello @o-nikolas , couldn't get to it, I will look into it tomorrow if that's okay.

Lovely, thanks for following up 😃

@vbottu vbottu force-pushed the aip67-kubernetes-executor-multi-team branch from 50fdb3f to 3cde943 Compare March 1, 2026 07:34
@vbottu
Copy link
Copy Markdown
Contributor Author

vbottu commented Mar 1, 2026

Thanks for the PR! A few comments. Also what kind of manual QA have you done?

Tested this end to end on a KinD cluster. I set up two team namespaces team-a-ns, team-b-ns with their own service accounts and configmaps, added the team records and bundle to team mappings in postgres, and configured the scheduler with the multi team env vars. Then I unpaused and triggered example_simplest_dag which sits on the dags-folder bundle mapped to team_a. The pod came up in team-a-ns, pulled the image, ran, and completed successfully. Confirmed it all with kubectl get events -n team-a-ns.

Copy link
Copy Markdown
Contributor

@jscheffl jscheffl left a comment

Choose a reason for hiding this comment

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

In general OK, just two nit in my second pass. After fixing these I think it is good to merge.

Copy link
Copy Markdown
Contributor

@o-nikolas o-nikolas left a comment

Choose a reason for hiding this comment

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

Other than Jen's feedback, lgtm. I re-triggered the failed builds since they don't immediately look related to me. Hopefully they are just flakey.

@jscheffl
Copy link
Copy Markdown
Contributor

jscheffl commented Mar 4, 2026

Other than Jen's feedback, lgtm. I re-triggered the failed builds since they don't immediately look related to me. Hopefully they are just flakey.

I assume the failures of the e2e UI tests are unrelated, but with next commit I assume / hope they will turn green. No need to worry about them

@o-nikolas
Copy link
Copy Markdown
Contributor

Other than Jen's feedback, lgtm. I re-triggered the failed builds since they don't immediately look related to me. Hopefully they are just flakey.

I assume the failures of the e2e UI tests are unrelated, but with next commit I assume / hope they will turn green. No need to worry about them

Looks like the new build failed the same tests. Bringing in changes from main to see if that changes anything!

@jscheffl
Copy link
Copy Markdown
Contributor

jscheffl commented Mar 5, 2026

Other than Jen's feedback, lgtm. I re-triggered the failed builds since they don't immediately look related to me. Hopefully they are just flakey.

I assume the failures of the e2e UI tests are unrelated, but with next commit I assume / hope they will turn green. No need to worry about them

Looks like the new build failed the same tests. Bringing in changes from main to see if that changes anything!

Hint: Running once prek run -a update-pyproject-toml will fix the problems, you can commit this and then I'd assume CI should turn green.

@vbottu
Copy link
Copy Markdown
Contributor Author

vbottu commented Mar 5, 2026

Sure , I will take a look , Thank you

@o-nikolas
Copy link
Copy Markdown
Contributor

Merging from main one last time. Should fix the apache-airflow-providers-informatica>=0.1.0 issue (Jarek fixed that in #62897)

@jscheffl
Copy link
Copy Markdown
Contributor

jscheffl commented Mar 7, 2026

Some comments are still open, I assusme they are not complex to fix. Would be cool to address them - then we can merge it before next provider wave.

@vbottu
Copy link
Copy Markdown
Contributor Author

vbottu commented Mar 7, 2026

Some comments are still open, I assusme they are not complex to fix. Would be cool to address them - then we can merge it before next provider wave.

working on them, will make the update soon

@jscheffl jscheffl merged commit 503b467 into apache:main Mar 7, 2026
252 of 253 checks passed
dominikhei pushed a commit to dominikhei/airflow that referenced this pull request Mar 11, 2026
* Add multi-team support for KubernetesExecutor

* Address review feedback and fix configuration fallback bug

* use AIRFLOW_V_3_2_PLUS version guard and narrow exception types

---------

Co-authored-by: Niko Oliveira <onikolas@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:providers full tests needed We need to run full set of tests for this PR to merge provider:cncf-kubernetes Kubernetes (k8s) provider related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants