-
Notifications
You must be signed in to change notification settings - Fork 5.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Culling: ensure last_activity attr exists before use #5355
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -459,9 +459,9 @@ async def cull_kernel_if_idle(self, kernel_id): | |
except KeyError: | ||
return # KeyErrors are somewhat expected since the kernel can be shutdown as the culling check is made. | ||
|
||
self.log.debug("kernel_id=%s, kernel_name=%s, last_activity=%s", | ||
kernel_id, kernel.kernel_name, kernel.last_activity) | ||
if kernel.last_activity is not None: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if |
||
if hasattr(kernel, 'last_activity'): # last_activity is monkey-patched, so ensure that has occurred | ||
self.log.debug("kernel_id=%s, kernel_name=%s, last_activity=%s", | ||
kernel_id, kernel.kernel_name, kernel.last_activity) | ||
dt_now = utcnow() | ||
dt_idle = dt_now - kernel.last_activity | ||
# Compute idle properties | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,7 @@ | |
import sys | ||
import time | ||
|
||
from requests import HTTPError | ||
from traitlets.config import Config | ||
|
||
from tornado.httpclient import HTTPRequest | ||
|
@@ -234,3 +235,49 @@ class KernelFilterTest(NotebookTestBase): | |
# Sanity check verifying that the configurable was properly set. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 235 |
||
def test_config(self): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 236 |
||
self.assertEqual(self.notebook.kernel_manager.allowed_message_types, ['kernel_info_request']) | ||
|
||
|
||
class KernelCullingTest(NotebookTestBase): | ||
"""Test kernel culling """ | ||
|
||
@classmethod | ||
def get_argv(cls): | ||
argv = super(KernelCullingTest, cls).get_argv() | ||
|
||
# Enable culling with 2s timeout and 1s intervals | ||
argv.extend(['--MappingKernelManager.cull_idle_timeout=2', | ||
'--MappingKernelManager.cull_interval=1', | ||
'--MappingKernelManager.cull_connected=False']) | ||
return argv | ||
|
||
def setUp(self): | ||
self.kern_api = KernelAPI(self.request, | ||
base_url=self.base_url(), | ||
headers=self.auth_headers(), | ||
) | ||
|
||
def tearDown(self): | ||
for k in self.kern_api.list().json(): | ||
self.kern_api.shutdown(k['id']) | ||
|
||
def test_culling(self): | ||
kid = self.kern_api.start().json()['id'] | ||
ws = self.kern_api.websocket(kid) | ||
model = self.kern_api.get(kid).json() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Heads-up: this test has a race condition. On slower hardware the kernel is already culled by the time the model is fetched and the assertion below fails. Example build failure: https://hydra.nixos.org/build/134833130 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you Matej. Yeah, I raised this suspicion in the jupyter_server meeting the other day and believe 2 seconds is a bit short. I think extending the idle timeout to 5 seconds - given this is the only test like this - will be sufficient. |
||
self.assertEqual(model['connections'], 1) | ||
assert not self.get_cull_status(kid) # connected, should not be culled | ||
ws.close() | ||
assert self.get_cull_status(kid) # not connected, should be culled | ||
|
||
def get_cull_status(self, kid): | ||
culled = False | ||
for i in range(15): # Need max of 3s to ensure culling timeout exceeded | ||
try: | ||
self.kern_api.get(kid) | ||
except HTTPError as e: | ||
assert e.response.status_code == 404 | ||
culled = True | ||
break | ||
else: | ||
time.sleep(0.2) | ||
return culled |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.