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

CLI 'kubernetes cleanup-pods' should only clean up Airflow-created Pods #15193

Closed
XD-DENG opened this issue Apr 4, 2021 · 7 comments · Fixed by #15204
Closed

CLI 'kubernetes cleanup-pods' should only clean up Airflow-created Pods #15193

XD-DENG opened this issue Apr 4, 2021 · 7 comments · Fixed by #15204
Labels
area:CLI kind:bug This is a clearly a bug provider:cncf-kubernetes Kubernetes provider related issues

Comments

@XD-DENG
Copy link
Member

XD-DENG commented Apr 4, 2021

Update

We discussed and decide this CLI should always only clean up pods created by Airflow.


Description

Currently command kubernetes cleanup-pods cleans up Pods that meet specific conditions in a given namespace.

Underlying code: https://github.com/apache/airflow/blob/2.0.1/airflow/cli/commands/kubernetes_command.py#L70

Use case / motivation

The problem to me is: users may have other non-Airflow stuff running in this specific namespace, and they may want to have different strategy/logic for Pod cleaning-up for these non-Airflow pods.

We may want to have an additional boolean flag for this command, like --only-airflow-pods, so that users can decide if they only want to clean up Airflow-created Pods.

It should not be hard to identify Airflow-created pods, given the specific labels Airflow adds to the Pods it creates (https://github.com/apache/airflow/blob/2.0.1/airflow/kubernetes/pod_generator.py#L385)

@XD-DENG XD-DENG added area:CLI kind:feature Feature Requests provider:cncf-kubernetes Kubernetes provider related issues labels Apr 4, 2021
@XD-DENG
Copy link
Member Author

XD-DENG commented Apr 4, 2021

@dimberman @mik-laj may you please share your inputs on if this makes sense to you or not? Let me know if I overlooked anything. Thanks

@XD-DENG
Copy link
Member Author

XD-DENG commented Apr 5, 2021

CC @kaxil @ashb

@mik-laj
Copy link
Member

mik-laj commented Apr 5, 2021

in my opinion this is a bug and by default we should only manage pods created by Airflow i.e. we don't need extra option.

@XD-DENG
Copy link
Member Author

XD-DENG commented Apr 5, 2021

That's my first thought as well, but I would like to hear the original author's opinion, like if there is any specific reason or not

This feature was added in #11802 . @kaxil may you please share your inputs? Cheers!

@potiuk
Copy link
Member

potiuk commented Apr 5, 2021

in my opinion this is a bug and by default we should only manage pods created by Airflow i.e. we don't need extra option.

My thoughts exactly!

@kaxil
Copy link
Member

kaxil commented Apr 5, 2021

Hey, thanks for this issue @XD-DENG . I added this feature and we use it in Astronomer too -- to cleanup any hanging PODs and their sidecars. So yeah we can just restrict it to pod launched via Airflow.

@XD-DENG
Copy link
Member Author

XD-DENG commented Apr 5, 2021

Thanks everyone.

I think we have come to a consensus. So I will change kind: feature to kind: bug and work on this later.

@XD-DENG XD-DENG added kind:bug This is a clearly a bug and removed kind:feature Feature Requests labels Apr 5, 2021
@XD-DENG XD-DENG changed the title CLI kubernetes cleanup-pods should allow "only clean up Pods created by Airflow" CLI 'kubernetes cleanup-pods' should only clean up Airflow-created Pods Apr 5, 2021
XD-DENG added a commit that referenced this issue Apr 8, 2021
…eated Pods (#15204)

closes: #15193

Currently condition if the pod is created by Airflow is not considered. This commit fixes this.

We decide if the Pod is created by Airflow via checking if it has all the labels added in PodGenerator.construct_pod() or KubernetesPodOperator.create_labels_for_pod().
ashb pushed a commit that referenced this issue Apr 15, 2021
…eated Pods (#15204)

closes: #15193

Currently condition if the pod is created by Airflow is not considered. This commit fixes this.

We decide if the Pod is created by Airflow via checking if it has all the labels added in PodGenerator.construct_pod() or KubernetesPodOperator.create_labels_for_pod().

(cherry picked from commit c594d9c)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:CLI kind:bug This is a clearly a bug provider:cncf-kubernetes Kubernetes provider related issues
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants