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

Delete pods by default in KubernetesPodOperator #20575

Merged
merged 4 commits into from
Dec 31, 2021

Conversation

dstandish
Copy link
Contributor

We change the default for is_delete_operator_pod to True. For subclasses GKEStartPodOperator and EksPodOperator we do not yet change the default since we may not want to do a major release in those providers. Instead we identify when the parameter is not set and emit a deprecation warning to notify users of the impending change.

@boring-cyborg boring-cyborg bot added provider:cncf-kubernetes Kubernetes provider related issues area:providers provider:amazon-aws AWS/Amazon - related issues provider:google Google (including GCP) related issues labels Dec 30, 2021
@dstandish
Copy link
Contributor Author

ready for [re]perusal

@potiuk
Copy link
Member

potiuk commented Dec 30, 2021

Some static checks fail.

@github-actions
Copy link

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 main or amend the last commit of the PR, and push it with --force-with-lease.

@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 Dec 30, 2021
We change the default for `is_delete_operator_pod` to `True`.  For subclasses `GKEStartPodOperator` and `EksPodOperator` we do not _yet_ change the default since we may not want to do a major release in those providers.  Instead we identify when the parameter is not set and emit a deprecation warning to notify users of the impending change.
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.

Build and test provider packages wheel failed with the following:

ERROR! There were 2 warnings generated during the import

Ideally, fix it, so that no warnings are generated during import.
There are two cases that are legitimate deprecation warnings though:
 1) when you deprecate whole module or class and replace it in provider
 2) when 3rd-party module generates Deprecation and you cannot upgrade it
 3) when many 3rd-party module generates same Deprecation warning that comes from another common library

In case 1), add the deprecation message to the KNOWN_DEPRECATED_DIRECT_IMPORTS in prepare_provider_packages.py
In case 2), add the deprecation message together with module it generates to the KNOWN_DEPRECATED_MESSAGES in prepare_provider_packages.py
In case 3), add the deprecation message to the KNOWN_COMMON_DEPRECATED_MESSAGES in prepare_provider_packages.py

@dstandish
Copy link
Contributor Author

Build and test provider packages wheel failed with the following:

This was due to an example dag which instantiated the GKE subclass without specifying a default for is_delete_operator_pod (which we're warning about now since the default has changed in KPO).

I decided to resolved this by setting explicit value for is_delete_operator_pod=True in the example dag, to be explicit and update it to be the new default (previously the behavior would have been False).

in this commit: 8c6a574

@potiuk potiuk merged commit 746ee58 into apache:main Dec 31, 2021
@potiuk
Copy link
Member

potiuk commented Dec 31, 2021

I decided to resolved this by setting explicit value for is_delete_operator_pod=True in the example dag, to be explicit and update it to be the new default (previously the behavior would have been False).

I really like how our tests are catching those cases. Because we actually found this way that our example docs were giving bad advice to our users (after the change) and we also improved the advice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:providers okay to merge It's ok to merge this PR as it does not require more tests provider:amazon-aws AWS/Amazon - related issues provider:cncf-kubernetes Kubernetes provider related issues provider:google Google (including GCP) related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants