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

New configurable/overridable kernel ZMQ+Websocket connection API #1047

Merged
merged 9 commits into from
Nov 20, 2022

Conversation

Zsailer
Copy link
Member

@Zsailer Zsailer commented Nov 3, 2022

Summary

Decouples the ZMQ socket handling logic from the websocket handler definition to enable server deployers to override this logic without forking the websocket handler.

The Problem

The logic for connecting a websocket to a kernel's set of ZMQ sockets is defined under methods in kernel's websocket handler.

This presents the following problem:

If you need to change, override, and remove logic underneath the kernel's /api/kernels/<kernel-id>/channels handler, you are forced to 1) fork Jupyter Server's ZMQChannelshandler and 2) insert the forked version earlier in the list of Jupyter Server's handlers to ensure it intercepts all kernel channels requests.

(This PR was inspired by some work I have been doing, experimenting with adding events and additional logging to the kernel's websocket API and found it impossible to do this without forking.)

Proposed solution

Separate the "business" logic into an overrideable, configurable class.

In other services, Jupyter Server uses a different pattern than the current websocket handler; it decouples "business" logic from the REST API handlers/definitions by defining a configurable, overrideable "manager" that follows a defined interface and handles the "business" logic underneath the REST API handlers. You can completely override the logic in the managers, as long as they follow the defined manager interface (usually an ABC).

In this PR, I've defined a new abstraction/interface called a KernelWebsocketConnectionABC. The interface is minimal and clearly maps to the methods in Tornado's default WebsocketHandler.

I've also added a base class that handles the instantiation of Jupyter Client Session objects.

Then, the default implementation, called ZMQChannelsWebsocketConnection includes all the methods previously found in the websocket handler.

A Note on Backwards Compatibility.

Most user/deployers will not be impacted by this change.

This impacts anyone who has forked the ZMQChannelsHandler, but it doesn't break their fork. Presumably, anyone who has forked this handler will still inject their handler before this new API is ever called. As a result, this change would leave them unaffected, but we should recommend that they update their code to use this new API.

Todo

  • Make prepare optional
  • Make open async
  • Make parent of the kernel websocket connection object the kernel_manager and parent.parent the multi_kernel_manager.
  • deprecate traits on ServerApp and move to ZMQChannelsWebsocketConnection
  • Get tests passing

@blink1073
Copy link
Contributor

Notes from today's discussion

  • prepare could be made optional
  • open can be async
  • parent could be the KernelManager

@Zsailer Zsailer changed the title Message broker API for configurable/overridable handling of ZMQ+Websocket messages New configurable/overridable kernel ZMQ+Websocket connection API Nov 7, 2022
@codecov
Copy link

codecov bot commented Nov 17, 2022

Codecov Report

Attention: Patch coverage is 55.61878% with 312 lines in your changes missing coverage. Please review.

Project coverage is 75.90%. Comparing base (e66306d) to head (ffdcef8).
Report is 292 commits behind head on main.

Files with missing lines Patch % Lines
...ter_server/services/kernels/connection/channels.py 47.48% 213 Missing and 27 partials ⚠️
jupyter_server/base/websocket.py 46.37% 36 Missing and 1 partial ⚠️
jupyter_server/services/kernels/connection/base.py 73.25% 22 Missing and 1 partial ⚠️
jupyter_server/services/kernels/websocket.py 83.33% 4 Missing and 3 partials ⚠️
jupyter_server/serverapp.py 87.50% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1047      +/-   ##
==========================================
- Coverage   76.28%   75.90%   -0.39%     
==========================================
  Files          63       68       +5     
  Lines        8228     8305      +77     
  Branches     1637     1655      +18     
==========================================
+ Hits         6277     6304      +27     
- Misses       1546     1605      +59     
+ Partials      405      396       -9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@blink1073 blink1073 mentioned this pull request Nov 17, 2022
9 tasks
@blink1073
Copy link
Contributor

The macos failures are unrelated, I'll make them more robust in #1069. There are a number of typing failures though.

@Zsailer Zsailer marked this pull request as ready for review November 18, 2022 19:02
@Zsailer
Copy link
Member Author

Zsailer commented Nov 18, 2022

Bringing this out of draft state. I think it is ready for final review.

@blink1073 do you mind taking a look over?

@blink1073
Copy link
Contributor

Yep, I'll take a look, but probably not today.

jupyter_server/base/zmqhandlers.py Outdated Show resolved Hide resolved
jupyter_server/serverapp.py Show resolved Hide resolved
jupyter_server/services/kernels/connection/channels.py Outdated Show resolved Hide resolved
@Zsailer
Copy link
Member Author

Zsailer commented Nov 19, 2022

The Jupyter Server Terminals tests are failing because of the deprecation warning on the zmqhandlers module. I can remove this warning if preferred, or update jupyter_server_terminals to import from the new location, jupyter_server.services.kernels.websocket. What do you think?

@blink1073
Copy link
Contributor

Since the terminals plugin depends on 2.0, I'd say update there

@blink1073
Copy link
Contributor

But not until after the rc is released

@blink1073
Copy link
Contributor

Actually, it seems like we need to ignore the warning here, release the RC, release a fix in terminals, and then remove the warning ignore here.

@Zsailer
Copy link
Member Author

Zsailer commented Nov 20, 2022

Okay—I've ignored the warnings here. The downstream tests will still fail because we need to update jupyter_server terminals. I've opened this PR in jupyter_server_terminals to handle the new module.

If I understand correctly, we'll

  1. merge this PR even though the downstream tests are failing
  2. cut an RC with these changes
  3. kick CI in my PR in jupyter_server_terminals to pick up the new Jupyter Server rc.
  4. when it passes, merge that PR and cut a release of jupyter_server_terminals.

@blink1073
Copy link
Contributor

Yep, sounds good!

@blink1073
Copy link
Contributor

Okay, I'll cut an RC tomorrow with this. We can still target 5 Dec for the final since a lot of folks will be out of office this week.

Copy link
Contributor

@blink1073 blink1073 left a comment

Choose a reason for hiding this comment

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

Thanks!

@blink1073 blink1073 merged commit 884b72e into jupyter-server:main Nov 20, 2022
@bloomsa
Copy link
Contributor

bloomsa commented Nov 21, 2022

I was just looking at overriding the /kernel-id/channels endpoint to do a check that the user requesting the WS was the user that owned the kernel. great timing on this abstraction piece! thank you

@bloomsa
Copy link
Contributor

bloomsa commented Nov 21, 2022

I was just looking at overriding the /kernel-id/channels endpoint to do a check that the user requesting the WS was the user that owned the kernel. great timing on this abstraction piece! thank you

shoot, I was hoping to use this with Enterprise-gateway but conflicting deps. force me to use a 1.x version of jupyter-server. Is a backport of this to 1.x possible, or does it depend on other 2.0 functionality? @Zsailer (not asking you to do the work, I can try to implement it, just wondering about the feasibility)

@kevin-bates
Copy link
Member

Hi @bloomsa. (I am "out of the office" this week, as, I believe, is @Zsailer - just fyi.)

My understanding is that this feature is intended for 2.0+. Should a backport to 1.x be considered, it must not introduce a dependency on jupyter_client 7+ (which is currently the reason EG needs to cap Server < 2) since current EG releases (<= 3.0) require jupyter_client < 7. That is, the dependency ice on which EG rests is extremely thin! 😄

I think it may make more sense to use this feature (which I agree is great!) as another impetus to getting EG 4.0 going where we plan to derive EG directly from ServerApp along with Remote Provisioners to continue supporting resource-managed kernels. Derivation from ServerApp would then enable EG to support things like server extensions etc. We could then repurpose EG functionality like Kernelspec Caching, Dynamic Configurables, and applicable portions of the High Availability functionality as server extensions while moving us closer to service composability.

@bloomsa
Copy link
Contributor

bloomsa commented Nov 22, 2022

@kevin-bates I appreciate the reply, especially while OOO! My use case for this is to ensure that the person trying to connect to a kernel with the api/kernels/kernel-id/channels is the same as the user that created the kernel in the first place. Preventing a situation where someone can access a kernel and its state when they aren't supposed to. I think I am targeting the right spot, being the ZMQChannelsHandler, with overriding its get mapping method to validate a user. LMK if this is something the Jupyter team has already considered or if another approach would make more sense

@kevin-bates
Copy link
Member

I'm not sure how much something like this will interfere with RTC (real-time collaboration) aside from my concern about EG's precarious dependency position - so I'm more concerned about when and where than how. I think it's great that you're looking into this, but we should also keep in mind the other areas of recent activity like RTC and authorization. Unfortunately, I'm not intimately familiar with RTC or the authorization work (I apologize).

I'm assuming your changes would be directed at the EG server itself, not the EG gateway client package built into ServerApp - correct?

Once EG 4.0 is in place (deriving from or at least closer to ServerApp and no ceiling on jupyter_client) we would be in a better position to leverage these kinds of things.

@bloomsa
Copy link
Contributor

bloomsa commented Dec 2, 2022

@kevin-bates I hadn't considered the interference with RTC, good point. In the end, I was able to override the ZMQChannelsHandler method simply by making a child class off of ZMQChannelsHandler and overriding the get method in my Enterprise Gateway implementation. It worked similar to how the kernel handlers are overridden with Mixins right now. So there was no need for this to be backported after all 👍

@kevin-bates
Copy link
Member

Great -thanks for getting back to us @bloomsa.

@echarles
Copy link
Member

echarles commented Dec 3, 2022

I think it may make more sense to use this feature (which I agree is great!) as another impetus to getting EG 4.0 going where we plan to derive EG directly from ServerApp along with Remote Provisioners

Is there an issue that tracks EG on ServerApp + RemoteProvisionner?

@kevin-bates
Copy link
Member

@echarles - It's vaguely on the EG project roadmap but it's probably time we can open an issue or two (in EG).

@echarles
Copy link
Member

echarles commented Dec 3, 2022

@kevin-bates jupyter-server/enterprise_gateway#1208

ojarjur added a commit to ojarjur/jupyter_server that referenced this pull request Apr 20, 2023
The two `WebSocketChannelsHandler` and `GatewayResourceHandler` classes are removed and
their corresponding functionality is merged into the respective `KernelWebsocketHandler`
and `KernelSpecResourceHandler` classes.

For the `KernelSpecResourceHandler` class, this change is rather straightforward as
we can simply make the existing handler check if the kernel spec manager has a
`get_kernel_spec_resource` method, and if so delegate to that method instead of
trying to read resources from disk.

The `KernelWebsocketHandler` conversion is more complicated, though. The handling of
websocket connections was generalized/extended in jupyter-server#1047 to allow the definition of
a `kernel_websocket_connection_class` as an alternative to replacing the entire
websocket handler.

This change builds on that by converting the `GatewayWebSocketClient` class to be
an instance of the kernel websocket connection class, and accordingly renames it to
`GatewayWebSocketConnection`.

When the gateway client is enabled, the default `kernel_websocket_connection_class`
is changed to this `GatewayWebSocketConnection` class similarly to how the kernel
and kernel spec manager default classes are updated.
@Zsailer Zsailer deleted the message-broker branch January 16, 2024 21:48
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.

5 participants