You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "Owen-CH-Leung (via GitHub)" <gi...@apache.org> on 2023/09/20 10:24:39 UTC
[GitHub] [airflow] Owen-CH-Leung opened a new pull request, #34500: Add remove pod logic
Owen-CH-Leung opened a new pull request, #34500:
URL: https://github.com/apache/airflow/pull/34500
fixes #34482
kubelet client will not remove pods for us. So after removing istio sidecar, we will have to explicitly remove the pod also.
Now when the base container return exit code 1 (i.e. error) and `delete_pod` is passed in, it will delete the pod after killing the sidecar. See below screen recording
https://github.com/apache/airflow/assets/43698890/657f51b9-5521-4309-925f-7df06603934e
And when the base container return exit code 1 and `delete_succeeded_pod` is passed in, it will not delete the pod.
https://github.com/apache/airflow/assets/43698890/c7af3e9f-2192-4e12-9e1e-6d24d5e7b810
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham merged pull request #34500: Fix Pod not being removed after istio-sidecar is removed
Posted by "jedcunningham (via GitHub)" <gi...@apache.org>.
jedcunningham merged PR #34500:
URL: https://github.com/apache/airflow/pull/34500
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Owen-CH-Leung commented on pull request #34500: Fix Pod not being removed after istio-sidecar is removed
Posted by "Owen-CH-Leung (via GitHub)" <gi...@apache.org>.
Owen-CH-Leung commented on PR #34500:
URL: https://github.com/apache/airflow/pull/34500#issuecomment-1727478468
Please don't merge this PR - my friend joshua has spotted some additional areas of improvement and will commit to this PR soon
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Owen-CH-Leung commented on pull request #34500: Fix Pod not being removed after istio-sidecar is removed
Posted by "Owen-CH-Leung (via GitHub)" <gi...@apache.org>.
Owen-CH-Leung commented on PR #34500:
URL: https://github.com/apache/airflow/pull/34500#issuecomment-1727487185
> > 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Owen-CH-Leung commented on a diff in pull request #34500: Fix Pod not being removed after istio-sidecar is removed
Posted by "Owen-CH-Leung (via GitHub)" <gi...@apache.org>.
Owen-CH-Leung commented on code in PR #34500:
URL: https://github.com/apache/airflow/pull/34500#discussion_r1336617496
##########
tests/providers/cncf/kubernetes/operators/test_pod.py:
##########
@@ -686,14 +684,6 @@ def test_pod_with_istio_delete_after_await_container_error(
k.execute(context=context)
assert is_istio_enabled_mock(find_pod_mock.return_value)
Review Comment:
Yes you are right. I've revised the test to check if `delete_pod` is called
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] hussein-awala commented on pull request #34500: Fix Pod not being removed after istio-sidecar is removed
Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala commented on PR #34500:
URL: https://github.com/apache/airflow/pull/34500#issuecomment-1727482655
> 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] hussein-awala commented on a diff in pull request #34500: Fix Pod not being removed after istio-sidecar is removed
Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala commented on code in PR #34500:
URL: https://github.com/apache/airflow/pull/34500#discussion_r1335236053
##########
airflow/providers/cncf/kubernetes/operators/pod.py:
##########
@@ -822,8 +823,12 @@ def process_pod_deletion(self, pod: k8s.V1Pod, *, reraise=True):
self.log.info("Deleting pod: %s", pod.metadata.name)
self.pod_manager.delete_pod(pod)
elif should_delete_pod and istio_enabled:
- self.log.info("Deleting istio-proxy sidecar inside %s: ", pod.metadata.name)
- self.kill_istio_sidecar(pod)
+ if container_is_running(pod, self.ISTIO_CONTAINER_NAME):
+ self.log.info("Deleting istio-proxy sidecar inside %s: ", pod.metadata.name)
+ self.kill_istio_sidecar(pod)
+ self.pod_manager.delete_pod(pod)
+ else:
+ self.pod_manager.delete_pod(pod)
Review Comment:
Indeed, when we attempt to terminate the pod, all the containers receive a SIGTERM signal. They manage this through a signal handler. Kubernetes will wait a maximum of `graceperiod` seconds before resorting to a SIGKILL to forcefully terminate them. Given that the KPO has the `termination_grace_period` argument, I concur with Jed's suggestion with a small change; no need for `if should_delete_pod and not istio_enabled` and `if should_delete_pod and istio_enabled`, all this block could be deleted, and we can kill the pod when `should_delete_pod` is Truee regardless the value of `istio_enabled`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a diff in pull request #34500: Fix Pod not being removed after istio-sidecar is removed
Posted by "jedcunningham (via GitHub)" <gi...@apache.org>.
jedcunningham commented on code in PR #34500:
URL: https://github.com/apache/airflow/pull/34500#discussion_r1336172010
##########
tests/providers/cncf/kubernetes/operators/test_pod.py:
##########
@@ -686,14 +684,6 @@ def test_pod_with_istio_delete_after_await_container_error(
k.execute(context=context)
assert is_istio_enabled_mock(find_pod_mock.return_value)
Review Comment:
We should either get rid of this test, or test that delete is called. Right now we don't check anything of value any longer, right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Owen-CH-Leung commented on pull request #34500: Fix Pod not being removed after istio-sidecar is removed
Posted by "Owen-CH-Leung (via GitHub)" <gi...@apache.org>.
Owen-CH-Leung commented on PR #34500:
URL: https://github.com/apache/airflow/pull/34500#issuecomment-1729052402
@hussein-awala Can I seek for your review again ? Many thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Owen-CH-Leung commented on pull request #34500: Fix Pod not being removed after istio-sidecar is removed
Posted by "Owen-CH-Leung (via GitHub)" <gi...@apache.org>.
Owen-CH-Leung commented on PR #34500:
URL: https://github.com/apache/airflow/pull/34500#issuecomment-1736712901
> Can you update the summary/description of this PR now that the approach has changed a bit?
Thanks. I've updated the descriptions
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Owen-CH-Leung commented on a diff in pull request #34500: Fix Pod not being removed after istio-sidecar is removed
Posted by "Owen-CH-Leung (via GitHub)" <gi...@apache.org>.
Owen-CH-Leung commented on code in PR #34500:
URL: https://github.com/apache/airflow/pull/34500#discussion_r1336112368
##########
airflow/providers/cncf/kubernetes/operators/pod.py:
##########
@@ -822,8 +823,12 @@ def process_pod_deletion(self, pod: k8s.V1Pod, *, reraise=True):
self.log.info("Deleting pod: %s", pod.metadata.name)
self.pod_manager.delete_pod(pod)
elif should_delete_pod and istio_enabled:
- self.log.info("Deleting istio-proxy sidecar inside %s: ", pod.metadata.name)
- self.kill_istio_sidecar(pod)
+ if container_is_running(pod, self.ISTIO_CONTAINER_NAME):
+ self.log.info("Deleting istio-proxy sidecar inside %s: ", pod.metadata.name)
+ self.kill_istio_sidecar(pod)
+ self.pod_manager.delete_pod(pod)
+ else:
+ self.pod_manager.delete_pod(pod)
Review Comment:
Thanks @hussein-awala and @jedcunningham for your feedback. I've adopted the suggestion and proceed for deletion whenever `should_delete_pod` is true.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a diff in pull request #34500: Fix Pod not being removed after istio-sidecar is removed
Posted by "jedcunningham (via GitHub)" <gi...@apache.org>.
jedcunningham commented on code in PR #34500:
URL: https://github.com/apache/airflow/pull/34500#discussion_r1334817805
##########
airflow/providers/cncf/kubernetes/operators/pod.py:
##########
@@ -822,8 +823,12 @@ def process_pod_deletion(self, pod: k8s.V1Pod, *, reraise=True):
self.log.info("Deleting pod: %s", pod.metadata.name)
self.pod_manager.delete_pod(pod)
elif should_delete_pod and istio_enabled:
- self.log.info("Deleting istio-proxy sidecar inside %s: ", pod.metadata.name)
- self.kill_istio_sidecar(pod)
+ if container_is_running(pod, self.ISTIO_CONTAINER_NAME):
+ self.log.info("Deleting istio-proxy sidecar inside %s: ", pod.metadata.name)
+ self.kill_istio_sidecar(pod)
+ self.pod_manager.delete_pod(pod)
+ else:
+ self.pod_manager.delete_pod(pod)
Review Comment:
```suggestion
self.pod_manager.delete_pod(pod)
```
Doesn't the sidecar listen to sigterm from k8s? In fact this whole thing can be simplified further, if so (no need to see if an istio sidecar is running).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Owen-CH-Leung commented on a diff in pull request #34500: Fix Pod not being removed after istio-sidecar is removed
Posted by "Owen-CH-Leung (via GitHub)" <gi...@apache.org>.
Owen-CH-Leung commented on code in PR #34500:
URL: https://github.com/apache/airflow/pull/34500#discussion_r1334939073
##########
airflow/providers/cncf/kubernetes/operators/pod.py:
##########
@@ -822,8 +823,12 @@ def process_pod_deletion(self, pod: k8s.V1Pod, *, reraise=True):
self.log.info("Deleting pod: %s", pod.metadata.name)
self.pod_manager.delete_pod(pod)
elif should_delete_pod and istio_enabled:
- self.log.info("Deleting istio-proxy sidecar inside %s: ", pod.metadata.name)
- self.kill_istio_sidecar(pod)
+ if container_is_running(pod, self.ISTIO_CONTAINER_NAME):
+ self.log.info("Deleting istio-proxy sidecar inside %s: ", pod.metadata.name)
+ self.kill_istio_sidecar(pod)
+ self.pod_manager.delete_pod(pod)
+ else:
+ self.pod_manager.delete_pod(pod)
Review Comment:
hmmm yes. In my istio version I can see that `delete_namespaced_pod` works even when sidecar is running, and seems to me that we don't actually need to `kill_istio_sidecar` at all. However I'm not sure if this behaviour also applies to older version of istio. Shall we keep the logic to end istio sidecar followed by deleting the pod ? Or we should just go ahead to delete the pod directly
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] joshuayeung commented on pull request #34500: Fix Pod not being removed after istio-sidecar is removed
Posted by "joshuayeung (via GitHub)" <gi...@apache.org>.
joshuayeung commented on PR #34500:
URL: https://github.com/apache/airflow/pull/34500#issuecomment-1727852754
If `istio-proxy` is not running (e.g. the pod is pending and the dag is timeout), `kill_istio_sidecar` will fail.
I added some code to check if istio is running
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on pull request #34500: Fix Pod not being removed after istio-sidecar is removed
Posted by "jedcunningham (via GitHub)" <gi...@apache.org>.
jedcunningham commented on PR #34500:
URL: https://github.com/apache/airflow/pull/34500#issuecomment-1735992311
Can you update the summary/description of this PR now that the approach has changed a bit?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Owen-CH-Leung commented on a diff in pull request #34500: Fix Pod not being removed after istio-sidecar is removed
Posted by "Owen-CH-Leung (via GitHub)" <gi...@apache.org>.
Owen-CH-Leung commented on code in PR #34500:
URL: https://github.com/apache/airflow/pull/34500#discussion_r1336112368
##########
airflow/providers/cncf/kubernetes/operators/pod.py:
##########
@@ -822,8 +823,12 @@ def process_pod_deletion(self, pod: k8s.V1Pod, *, reraise=True):
self.log.info("Deleting pod: %s", pod.metadata.name)
self.pod_manager.delete_pod(pod)
elif should_delete_pod and istio_enabled:
- self.log.info("Deleting istio-proxy sidecar inside %s: ", pod.metadata.name)
- self.kill_istio_sidecar(pod)
+ if container_is_running(pod, self.ISTIO_CONTAINER_NAME):
+ self.log.info("Deleting istio-proxy sidecar inside %s: ", pod.metadata.name)
+ self.kill_istio_sidecar(pod)
+ self.pod_manager.delete_pod(pod)
+ else:
+ self.pod_manager.delete_pod(pod)
Review Comment:
Thanks both for your feedback. I've adopted the suggestion and proceed for deletion whenever `should_delete_pod` is true.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org