Skip to content
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

Added new WebhookKernelSessionManager for Kernel Persistence #1101

Merged
merged 9 commits into from
Jun 10, 2022

Conversation

amazingarman
Copy link
Contributor

Added a new way to persist kernels to any database. You must create an API to interact with your database. The API should have 4 endpoints:

  • 1 DELETE that will delete all kernel sessions from a list of kernel ids
  • 1 POST that will take kernel id as a path variable and kernel session in body and save it to a database
  • 1 GET that will take the kernel id as a path variable and retrieve that information from a database
  • 1 GET that will retrieve a list of all kernel sessions from a database

In order to setup the WebhookKernelSessionManager, you must set --EnterpriseGatewayApp.kernel_session_manager_class to enterprise_gateway.services.sessions.kernelsessionmanager.WebhookKernelSessionManager, --WebhookKernelSessionManager.webhook_url to the endpoint url of your API and --WebhookKernelSessionManager.enable_persistence=True to enable kernel persistence.

@kevin-bates
Copy link
Member

This is very interesting @amazingarman! I find this to be a clever (and flexible) way to integrate with a persistent store of any kind. Very nice.

Here are some next steps that should probably be included in this pull request.

  1. Extend the list of classes to include WebhookKernelSessionManager. This will enable the configuration options to appear when running jupyter enterprisegateway --help-all.
  2. Extend the command-line options portion of the doc to include the WebhookKernelSessionManager options following the existing FileKernelSessionManager options.
  3. We should probably add a section in the Operators Guide for "Kernel Session Persistence" that includes bothFileKernelSessionManager and WebhookKernelSessionManager. This will have some overlap with the Availability Modes PR Availability modes #1095 where it defines the modes and talks a bit about session persistence.
  4. I'm wondering if we should introduce version info into the URL, but I suppose that would be up to the provider and built into the web-url. The concern being any extensions to the payload (like the addition of tenant ID in Add support for Tenant ID #1097).

On that "modes" topic, this PR implies you must be using some form of availability and I'm curious what your thoughts are about where we're headed with that? What kinds of issues are you running into with the current code base? (Mostly curious about re-connects and culling, but any other experiences would be very helpful.)

@amazingarman
Copy link
Contributor Author

@kevin-bates updated the PR based off of your comments.

  1. I'm wondering if we should introduce version info into the URL, but I suppose that would be up to the provider and built into the web-url. The concern being any extensions to the payload (like the addition of tenant ID in Add support for Tenant ID #1097).

I agree that the version info in the URL is up to the provider to figure out.

On that "modes" topic, this PR implies you must be using some form of availability and I'm curious what your thoughts are about where we're headed with that? What kinds of issues are you running into with the current code base? (Mostly curious about re-connects and culling, but any other experiences would be very helpful.)

I don't have any availability set up currently and in the early stages of building this out. The modes PR is something that will definitely be used and agree on the direction it seems to be heading in. The code base is easy to read and follow, although the documentation is sparse in areas. We found solutions reading through past PRs and Issues raised.

Copy link
Member

@kevin-bates kevin-bates left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amazingarman - thanks so much for the update. There are just a few minor touchups necessary, but this looks good!

We will have some heavy overlap with the documentation in #1095, but I'm happy to tackle that once this PR is merged (following the additional updates).

Thanks again!

docs/source/operators/config-kernel-persistence.md Outdated Show resolved Hide resolved
docs/source/operators/config-cli.md Outdated Show resolved Hide resolved
@amazingarman amazingarman requested a review from kevin-bates June 8, 2022 19:10
Copy link
Member

@kevin-bates kevin-bates left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amazingarman - Thank you for the updates and this nice feature!

@kevin-bates
Copy link
Member

Will merge Friday morning barring any objections.

@kevin-bates kevin-bates merged commit d167159 into jupyter-server:main Jun 10, 2022
@welcome
Copy link

welcome bot commented Jun 10, 2022

Congrats on your first merged pull request in this project! 🎉
congrats
Thank you for contributing, we are very proud of you! ❤️

@kevin-bates kevin-bates mentioned this pull request Jun 10, 2022
@amazingarman amazingarman deleted the WebhookKernel branch June 10, 2022 20:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants