Installs a handler to delete active pods on termination #46

dleehr · 2019-02-08T21:18:04Z

Adds k8s.PodMonitor class that keeps track of submitted but not deleted pods
Adds k8s.delete_pods() function to be called on sigterm or if cwlmain() raises an exception
installs SIGTERM handler into main.main() to invoke k8s.delete_pods()
Addresses a couple inconsistencies in tests

Fixes #41

…received

- The pod object won't match the list, and the name must be unique anyways

- Forgot to change this, so the list wasn't cleared out on the first pass

…logging

Testing podmonitor separately would require a lot of shared patches/mocks as k8s, so I moved them back. - This reverts commit 9c96658. - This reverts commit 6540d48.

johnbradley

Question about a possible resource leak. A couple testing tweaks.
It would also be nice to have a comment explaining which parts are run in a background thread to help understanding of how these locks are being used - Is the main thread responsible for receiving the signal and doing the final pod cleanup while the background threads create and delete pods individually?

johnbradley · 2019-02-11T14:22:42Z

calrissian/k8s.py

@@ -46,6 +47,7 @@ def __init__(self):
    def submit_pod(self, pod_body):
        pod = self.core_api_instance.create_namespaced_pod(self.namespace, pod_body)
        log.info('Created k8s pod name {} with id {}'.format(pod.metadata.name, pod.metadata.uid))
+        PodMonitor.add(pod)


Is there a possibility that a SIGTERM arrives just before this method, the main thread runs delete_pods() and the pod created here is leaked?

Shouldn't we test that PodMonitor.add is called when users run submit_pod

johnbradley · 2019-02-11T14:35:04Z

tests/test_k8s.py

@@ -98,7 +97,8 @@ def test_wait_skips_pod_when_state_is_running(self, mock_watch, mock_get_namespa
        self.assertIsNotNone(kc.pod)

    @patch('calrissian.k8s.watch')
-    def test_wait_finishes_when_pod_state_is_terminated(self, mock_watch, mock_get_namespace, mock_client):
+    @patch('calrissian.k8s.PodMonitor')
+    def test_wait_finishes_when_pod_state_is_terminated(self, mock_podmonitor, mock_watch, mock_get_namespace, mock_client):


Shouldn't we test that PodMonitor.remove is called here?

- allows us to call functions while the lock is acquired, addressing feedback from review

dleehr · 2019-02-11T20:43:22Z

Thanks for the feedback @johnbradley. I made a few changes to guard against the case you described and added some documentation on threading assumptions.

johnbradley

LGTM

dleehr added 14 commits February 7, 2019 16:12

Adds PodMonitor to track submitted pods and delete them when SIGTERM …

32e7ef8

…received

Activate logging and try to debug missing pod

ebd9a17

Change PodMonitor's pod list to use names

91585f7

- The pod object won't match the list, and the name must be unique anyways

Update pod deletion to use pod name, handle exceptions

62170ea

Correct variable name after cleanup to pod_names

f29fd7d

- Forgot to change this, so the list wasn't cleared out on the first pass

Fix test_wait_finishes_when_pod_state_is_terminated and disable test …

2158448

…logging

Tests for delete_pod_name

9c3ffc9

Move PodMonitor to separate file

6540d48

Implement tests for podmonitor

9c96658

Update exception caught for load_incluster_config()

94f3fe7

Revert podmonitor move and tests

9eef15d

Testing podmonitor separately would require a lot of shared patches/mocks as k8s, so I moved them back. - This reverts commit 9c96658. - This reverts commit 6540d48.

Restore PodMonitor tests and fix other broken tests

222af42

Patch over delete_pods to avoid loading k8s config

50e2986

Implement signal handler tests for main, reformat tests_main

22f8ad9

dleehr requested a review from johnbradley February 8, 2019 21:20

johnbradley reviewed Feb 11, 2019

View reviewed changes

dleehr added 3 commits February 11, 2019 11:38

Rework PodMonitor to be a Context Manager

5c90766

- allows us to call functions while the lock is acquired, addressing feedback from review

Add suggested tests for PodMonitor add/remove, document classes

959dd6c

Documents threading expectations for signal handler

aaf132a

dleehr requested a review from johnbradley February 12, 2019 14:35

johnbradley approved these changes Feb 12, 2019

View reviewed changes

dleehr merged commit a2fcc7a into master Feb 12, 2019

dleehr deleted the 38-cleanup-on-terminate branch February 13, 2019 19:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Installs a handler to delete active pods on termination #46

Installs a handler to delete active pods on termination #46

dleehr commented Feb 8, 2019

johnbradley left a comment

johnbradley Feb 11, 2019

johnbradley Feb 11, 2019

johnbradley Feb 11, 2019

dleehr commented Feb 11, 2019

johnbradley left a comment

Installs a handler to delete active pods on termination #46

Installs a handler to delete active pods on termination #46

Conversation

dleehr commented Feb 8, 2019

johnbradley left a comment

Choose a reason for hiding this comment

johnbradley Feb 11, 2019

Choose a reason for hiding this comment

johnbradley Feb 11, 2019

Choose a reason for hiding this comment

johnbradley Feb 11, 2019

Choose a reason for hiding this comment

dleehr commented Feb 11, 2019

johnbradley left a comment

Choose a reason for hiding this comment