[GitHub] [airflow] dstandish commented on a diff in pull request #22092: Find Pod Before Cleanup In KubernetesPodOperator Execution

[GitBox <gi...@apache.org>]

dstandish commented on code in PR #22092:
URL: https://github.com/apache/airflow/pull/22092#discussion_r874167700


##########
airflow/providers/cncf/kubernetes/operators/kubernetes_pod.py:
##########
@@ -418,8 +420,9 @@ def cleanup(self, pod: k8s.V1Pod, remote_pod: k8s.V1Pod):
                 with _suppress(Exception):
                     for event in self.pod_manager.read_pod_events(pod).items:
                         self.log.error("Pod Event: %s - %s", event.reason, event.message)
-            with _suppress(Exception):
-                self.process_pod_deletion(pod)
+            if remote_pod is not None:
+                with _suppress(Exception):
+                    self.process_pod_deletion(pod)

Review Comment:
   if we're only deleting when there's a `remote_pod`, we might as well use that obj in the deletion.
   
   i.e. 
   ```suggestion
                       self.process_pod_deletion(remote_pod)
   ```
   
   separately, can you modify `process_pod_deletion` to just return when `remote_pod` is `None` so you don't have to check everywhere you use it?



##########
airflow/providers/cncf/kubernetes/operators/kubernetes_pod.py:
##########
@@ -437,16 +439,18 @@ def cleanup(self, pod: k8s.V1Pod, remote_pod: k8s.V1Pod):
                 with _suppress(Exception):
                     for event in self.pod_manager.read_pod_events(pod).items:
                         self.log.error("Pod Event: %s - %s", event.reason, event.message)
-            with _suppress(Exception):
-                self.process_pod_deletion(pod)
+            if remote_pod is not None:
+                with _suppress(Exception):
+                    self.process_pod_deletion(pod)
             error_message = get_container_termination_message(remote_pod, self.BASE_CONTAINER_NAME)
             error_message = "\n" + error_message if error_message else ""
             raise AirflowException(
                 f'Pod {pod and pod.metadata.name} returned a failure:{error_message}\n{remote_pod}'
             )
         else:
-            with _suppress(Exception):
-                self.process_pod_deletion(pod)
+            if remote_pod is not None:

Review Comment:
   same comment as line 444



##########
tests/providers/cncf/kubernetes/operators/test_kubernetes_pod.py:
##########
@@ -270,7 +270,10 @@ def test_image_pull_policy_correctly_set(self):
         assert pod.spec.containers[0].image_pull_policy == "Always"
 
     @mock.patch("airflow.providers.cncf.kubernetes.utils.pod_manager.PodManager.delete_pod")
-    def test_pod_delete_even_on_launcher_error(self, delete_pod_mock):
+    @mock.patch("airflow.providers.cncf.kubernetes.operators.kubernetes_pod.KubernetesPodOperator.find_pod")
+    def test_pod_delete_even_on_launcher_error(self, find_pod_mock, delete_pod_mock):
+        remote_pod_mock = MagicMock()
+        find_pod_mock.return_value = remote_pod_mock

Review Comment:
   are these lines really needed?  doesn't seem we do anythin with them.



-- 
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