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

Swap internal RPC server for API server in the helm chart #44463

Merged
merged 2 commits into from
Nov 29, 2024

Conversation

ashb
Copy link
Member

@ashb ashb commented Nov 28, 2024

Previously the PRC server was possible to disable/enable, where as now the new
api-server is a required component in Airflow 3.0+

This also does a little bit of drive-by refactoring of the helper templates
for service account name generation

I'm not 100% sure what "default" version is the tests, so maybe me adding a parameterization for "3.0.0" is not right. I also don't know what version we set it the kube/helm tests we run against main etc.

Part of #44436 (and needed to fix main after #44427 was merged)


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

@boring-cyborg boring-cyborg bot added the area:helm-chart Airflow Helm Chart label Nov 28, 2024
@ashb ashb mentioned this pull request Nov 28, 2024
90 tasks
@ashb ashb requested a review from potiuk November 28, 2024 21:44
@ashb
Copy link
Member Author

ashb commented Nov 28, 2024

Looks like I've got a lot more tests to fix up though!

@potiuk
Copy link
Member

potiuk commented Nov 28, 2024

Looks like I've got a lot more tests to fix up though!

The more he looked inside the more Piglet wasn’t there.

Copy link
Member

@kaxil kaxil left a comment

Choose a reason for hiding this comment

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

I think the failures are just in helm_tests/airflow_core/test_rpc_server.py

@ashb
Copy link
Member Author

ashb commented Nov 28, 2024

"Tests / Kubernetes tests / K8S System:LocalExecutor - false - v1.28.13 (pull_request)" is still failing which is really unfortunate, I was hoping that would pass.

@ashb ashb force-pushed the run-api-server-helm-chart branch from be9d396 to f752f1c Compare November 28, 2024 22:37
@amoghrajesh

This comment was marked as resolved.

@ashb
Copy link
Member Author

ashb commented Nov 29, 2024

Okay, the "Tests / Kubernetes tests / K8S System:LocalExecutor" tests aren't passing because we aren't passing an airflowVersion in the values.

@ashb ashb force-pushed the run-api-server-helm-chart branch 2 times, most recently from 2384924 to de9c587 Compare November 29, 2024 14:51
Previously the PRC server was possible to disable/enable, where as now the new
api-server is a required component in Airflow 3.0+

This also does a little bit of drive-by refactoring of the helper templates
for service account name generation
@ashb ashb force-pushed the run-api-server-helm-chart branch from de9c587 to 086e855 Compare November 29, 2024 16:45
@@ -54,6 +54,7 @@ def test_integration_run_dag(self):
timeout=300,
)

@pytest.mark.xfail(reason="https://github.com/apache/airflow/issues/44481 needs to be implemented")
Copy link
Member Author

@ashb ashb Nov 29, 2024

Choose a reason for hiding this comment

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

Please note, I've marked one test as XFail for now.

We merged a change to swap the LocalExecutor over to the new AIP-72 TaskSDK
code, and it wasn't complete enough to work with xcom. To unblock main we are
temporarirly marking that test as xfail, with a link to the issue to fix it.
@ashb ashb force-pushed the run-api-server-helm-chart branch from 086e855 to 3d9eb7b Compare November 29, 2024 16:58
@ashb ashb merged commit 288c87d into main Nov 29, 2024
91 checks passed
@ashb ashb deleted the run-api-server-helm-chart branch November 29, 2024 17:49
LefterisXefteris pushed a commit to LefterisXefteris/airflow that referenced this pull request Jan 5, 2025
* Swap internal RPC server for API server in the helm chart

Previously the PRC server was possible to disable/enable, where as now the new
api-server is a required component in Airflow 3.0+

This also does a little bit of drive-by refactoring of the helper templates
for service account name generation

* Mark the LocalExecutor test that uses example_xcom dag as xfail for now

We merged a change to swap the LocalExecutor over to the new AIP-72 TaskSDK
code, and it wasn't complete enough to work with xcom. To unblock main we are
temporarirly marking that test as xfail, with a link to the issue to fix it.
got686-yandex pushed a commit to got686-yandex/airflow that referenced this pull request Jan 30, 2025
* Swap internal RPC server for API server in the helm chart

Previously the PRC server was possible to disable/enable, where as now the new
api-server is a required component in Airflow 3.0+

This also does a little bit of drive-by refactoring of the helper templates
for service account name generation

* Mark the LocalExecutor test that uses example_xcom dag as xfail for now

We merged a change to swap the LocalExecutor over to the new AIP-72 TaskSDK
code, and it wasn't complete enough to work with xcom. To unblock main we are
temporarirly marking that test as xfail, with a link to the issue to fix it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:helm-chart Airflow Helm Chart
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants