From ab0246fdfb5470a360f92b7bf868f2ebfb30fe8a Mon Sep 17 00:00:00 2001 From: John Bradley Date: Tue, 20 Aug 2019 10:07:37 -0400 Subject: [PATCH 1/2] test to demonstrate ValueError not in list --- tests/test_k8s.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/tests/test_k8s.py b/tests/test_k8s.py index 320f5ab..b44a1dc 100644 --- a/tests/test_k8s.py +++ b/tests/test_k8s.py @@ -365,6 +365,16 @@ def test_delete_pods_calls_podmonitor(self, mock_pod_monitor): delete_pods() self.assertTrue(mock_pod_monitor.cleanup.called) + @patch('calrissian.k8s.KubernetesClient') + def test_remove_after_cleanup(self, mock_client): + # Depending upon timing cleanup may get called before we receive a remove pod event + pod = self.make_mock_pod('pod-123') + with PodMonitor() as monitor: + monitor.add(pod) + PodMonitor.cleanup() + with PodMonitor() as monitor: + monitor.remove(pod) + class CompletionResultTestCase(TestCase): From a249143087d3409414c474d92a04f0be8b794f3b Mon Sep 17 00:00:00 2001 From: John Bradley Date: Tue, 20 Aug 2019 10:13:51 -0400 Subject: [PATCH 2/2] fix not in list ValueError This error occured due to `cwlmain` within calrissian/main.py returning before all the threads were finished. So the delete_pods cleanup method was called then one of the threads processed a message about a cleaned up pod. Not sure of the underlying reason why cwlmain returned early. Perhaps there is another error that this bug is masking. --- calrissian/k8s.py | 7 +++++-- tests/test_k8s.py | 8 +++++++- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/calrissian/k8s.py b/calrissian/k8s.py index 22023f3..49bbc78 100644 --- a/calrissian/k8s.py +++ b/calrissian/k8s.py @@ -263,9 +263,12 @@ def add(self, pod): PodMonitor.pod_names.append(pod.metadata.name) def remove(self, pod): - log.info('PodMonitor removing {}'.format(pod.metadata.name)) # This has to look up the pod by something unique - PodMonitor.pod_names.remove(pod.metadata.name) + if pod.metadata.name in PodMonitor.pod_names: + log.info('PodMonitor removing {}'.format(pod.metadata.name)) + PodMonitor.pod_names.remove(pod.metadata.name) + else: + log.warning('PodMonitor {} has already been removed'.format(pod.metadata.name)) @staticmethod def cleanup(): diff --git a/tests/test_k8s.py b/tests/test_k8s.py index b44a1dc..3b4d629 100644 --- a/tests/test_k8s.py +++ b/tests/test_k8s.py @@ -366,7 +366,8 @@ def test_delete_pods_calls_podmonitor(self, mock_pod_monitor): self.assertTrue(mock_pod_monitor.cleanup.called) @patch('calrissian.k8s.KubernetesClient') - def test_remove_after_cleanup(self, mock_client): + @patch('calrissian.k8s.log') + def test_remove_after_cleanup(self, mock_log, mock_client): # Depending upon timing cleanup may get called before we receive a remove pod event pod = self.make_mock_pod('pod-123') with PodMonitor() as monitor: @@ -374,6 +375,11 @@ def test_remove_after_cleanup(self, mock_client): PodMonitor.cleanup() with PodMonitor() as monitor: monitor.remove(pod) + mock_log.info.assert_has_calls([ + call('PodMonitor adding pod-123'), + call('PodMonitor deleting pod pod-123'), + ]) + mock_log.warning.assert_called_with('PodMonitor pod-123 has already been removed') class CompletionResultTestCase(TestCase):