Skip to content

Fix Pod not being removed after istio-sidecar is removed - #34500

Merged
jedcunningham merged 12 commits into
apache:mainfrom
Owen-CH-Leung:Fix-KPO-not-removing-pod
Sep 27, 2023
Merged

Fix Pod not being removed after istio-sidecar is removed #34500
jedcunningham merged 12 commits into
apache:mainfrom
Owen-CH-Leung:Fix-KPO-not-removing-pod

Conversation

@Owen-CH-Leung

@Owen-CH-Leung Owen-CH-Leung commented Sep 20, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

fixes #34482

kubelet client will not remove pods for us. After discussions, it is discovered that all containers inside a Pod will be able to respond to a SIGTERM signal. So instead of going into the istio-proxy container and use the /quitquitquit endpoint to pause the sidecar, we can just use the api delete_namespaced_pod to delete the entire pod.

This PR therefore changes the logic to delete the pod directly regardless of whether istio is enabled

@boring-cyborg boring-cyborg Bot added area:providers provider:cncf-kubernetes Kubernetes (k8s) provider related issues labels Sep 20, 2023
@Owen-CH-Leung Owen-CH-Leung changed the title Add remove pod logic Fix Pod not being removed after istio-sidecar is removed Sep 20, 2023
@Owen-CH-Leung

Owen-CH-Leung commented Sep 20, 2023

Copy link
Copy Markdown
Contributor Author

Please don't merge this PR - my friend joshua has spotted some additional areas of improvement and will commit to this PR soon

@hussein-awala
hussein-awala marked this pull request as draft September 20, 2023 10:48
@hussein-awala

hussein-awala commented Sep 20, 2023

Copy link
Copy Markdown
Member

Please don't merge this PR - my friend joshua has spotted some additional areas of improvement and will commit to this PR soon

You should keep your PR as a draft until it is ready

@Owen-CH-Leung

Owen-CH-Leung commented Sep 20, 2023

Copy link
Copy Markdown
Contributor Author

Please don't merge this PR - my friend joshua has spotted some additional areas of improvement and will commit to this PR soon

You should keep your PR as a draft until it is ready

Yup - sorry for that

@joshuayeung

joshuayeung commented Sep 20, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

If istio-proxy is not running (e.g. the pod is pending for resource allocation but the dag is timeout), kill_istio_sidecar will fail.
I added some code to check if istio is running

@Owen-CH-Leung
Owen-CH-Leung marked this pull request as ready for review September 21, 2023 07:54
@Owen-CH-Leung

Owen-CH-Leung commented Sep 21, 2023

Copy link
Copy Markdown
Contributor Author

@hussein-awala Can I seek for your review again ? Many thanks

jedcunningham
jedcunningham reviewed Sep 22, 2023
View reviewed changes
Comment thread airflow/providers/cncf/kubernetes/operators/pod.py Outdated
jedcunningham
jedcunningham reviewed Sep 25, 2023
View reviewed changes
Comment thread tests/providers/cncf/kubernetes/operators/test_pod.py Outdated
@jedcunningham

jedcunningham commented Sep 26, 2023

Copy link
Copy Markdown
Member

Can you update the summary/description of this PR now that the approach has changed a bit?

@Owen-CH-Leung

Owen-CH-Leung commented Sep 27, 2023

Copy link
Copy Markdown
Contributor Author

Can you update the summary/description of this PR now that the approach has changed a bit?

Thanks. I've updated the descriptions

@jedcunningham
jedcunningham merged commit fb92ff8 into apache:main Sep 27, 2023
@eladkal eladkal mentioned this pull request Oct 13, 2023
Closed
98 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hussein-awala hussein-awala hussein-awala approved these changes
@jedcunningham jedcunningham jedcunningham approved these changes

Assignees

No one assigned

Labels

area:providers provider:cncf-kubernetes Kubernetes (k8s) provider related issues

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

KubernetesPodOperator shutting down istio sidecar but not deleting pod upon failure

4 participants

@Owen-CH-Leung @hussein-awala @joshuayeung @jedcunningham