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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
