Skip to content

Conversation

@moiseenkov
Copy link
Contributor

  • implement new operators: KubernetesInstallKueueOperator, KubernetesStartKueueJobOperator in cncf-kubernetes provider
  • refactor GKE operators in google provider in order to reduce complexity and code duplication

@boring-cyborg boring-cyborg bot added area:providers provider:cncf-kubernetes Kubernetes (k8s) provider related issues provider:google Google (including GCP) related issues labels Dec 2, 2024
@moiseenkov moiseenkov force-pushed the k8s_kueue_operators branch 2 times, most recently from 165f411 to f6100f4 Compare December 2, 2024 15:49
@moiseenkov
Copy link
Contributor Author

@eladkal , @potiuk,

This PR contains changes for both Google and cncf-kubernetes providers, and the Google provider depends on changes in cncf-kuberneets. For this reason I had to bump cncf-kubernetes version in Google provider's dependencies and it now breaks CI because it can't install cncf-kubernetes provider that wasn't released yet.
Therefore, should I split this PR, so we could release these changes gradually (cncf first, and google later) or we are OK with merging it all together now.

@moiseenkov moiseenkov requested a review from eladkal December 4, 2024 15:43
@potiuk
Copy link
Member

potiuk commented Dec 6, 2024

@eladkal , @potiuk,

This PR contains changes for both Google and cncf-kubernetes providers, and the Google provider depends on changes in cncf-kuberneets. For this reason I had to bump cncf-kubernetes version in Google provider's dependencies and it now breaks CI because it can't install cncf-kubernetes provider that wasn't released yet. Therefore, should I split this PR, so we could release these changes gradually (cncf first, and google later) or we are OK with merging it all together now.

This is one option yes. There is another option. You can add the cncf.kubernetes provider to the list of "chicken-egg" providers in src/airflow_breeze/global_constants.py -> similarly to what I've done in #44714 (about to be merged).

@potiuk
Copy link
Member

potiuk commented Dec 6, 2024

Also updated documentation about it here #44720

@eladkal
Copy link
Contributor

eladkal commented Dec 6, 2024

@eladkal , @potiuk,
This PR contains changes for both Google and cncf-kubernetes providers, and the Google provider depends on changes in cncf-kuberneets. For this reason I had to bump cncf-kubernetes version in Google provider's dependencies and it now breaks CI because it can't install cncf-kubernetes provider that wasn't released yet. Therefore, should I split this PR, so we could release these changes gradually (cncf first, and google later) or we are OK with merging it all together now.

This is one option yes. There is another option. You can add the cncf.kubernetes provider to the list of "chicken-egg" providers in src/airflow_breeze/global_constants.py -> similarly to what I've done in #44714 (about to be merged).

Why not just using AirflowOptionalProviderFeatureException?

@moiseenkov
Copy link
Contributor Author

moiseenkov commented Dec 6, 2024

This is one option yes. There is another option. You can add the cncf.kubernetes provider to the list of "chicken-egg" providers in src/airflow_breeze/global_constants.py -> similarly to what I've done in #44714 (about to be merged).

@potiuk , thank you for the suggestion! I followed this idea. But it seems to me that the new version of Kubernetes provider should be announced in provider.yaml anyway... I can update my PR once it happens.

Why not just using AirflowOptionalProviderFeatureException?

I've also considered this, but unfortunately provided changes aren't optional. For example the GKEStartKueueInsideClusterOperator now inherits from KubernetesInstallKueueOperator which was introduced in this PR. At the same time the GKEStartKueueInsideClusterOperator is a bit older and will be broken if the Kubernetes provider isn't upgraded. Thus I had to bump the cncf-kubernetes provider version in google provider's dependencies.

@moiseenkov moiseenkov force-pushed the k8s_kueue_operators branch 3 times, most recently from f67940c to dbb265e Compare December 6, 2024 13:30
@moiseenkov moiseenkov requested a review from potiuk December 6, 2024 15:00
@potiuk
Copy link
Member

potiuk commented Dec 6, 2024

Tests failing - breeze (easier to fix) and compatibility tests with airflow 2.8 - 2.9 (a bit more difficult). The way how to run compatibilty test is described in detail in https://github.com/apache/airflow/blob/main/contributing-docs/testing/unit_tests.rst#compatibility-provider-unit-tests-against-older-airflow-releases - with some examples how to deal with the tests.

@moiseenkov moiseenkov force-pushed the k8s_kueue_operators branch 4 times, most recently from 476b266 to b4c7a5a Compare December 10, 2024 17:07
KubernetesStartKueueJobOperator and refactor GKE operatores
@moiseenkov
Copy link
Contributor Author

Hi @potiuk ,
The CI is finally green. Thank you very much for your guidance!
The PR is ready for review and merging hopefully.

@potiuk
Copy link
Member

potiuk commented Dec 11, 2024

Cool. The good things that all deprecations have been removed already in the previous wave - so we will not have another bump in k8s provider MAJOR version, so this one is good to go.

@potiuk potiuk merged commit 8480460 into apache:main Dec 11, 2024
97 checks passed
ellisms pushed a commit to ellisms/airflow that referenced this pull request Dec 13, 2024
KubernetesStartKueueJobOperator and refactor GKE operatores
got686-yandex pushed a commit to got686-yandex/airflow that referenced this pull request Jan 30, 2025
KubernetesStartKueueJobOperator and refactor GKE operatores
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 (k8s) provider related issues provider:google Google (including GCP) related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants