jupyter-server-proxy icon indicating copy to clipboard operation
jupyter-server-proxy copied to clipboard

Add manager for tracking and terminating running server proxies

Open mahendrapaipuri opened this issue 3 years ago • 7 comments

This PR addresses part of enhancements proposed in #383

Backend Here we are adding a manager on the backend to keep track of running server proxy apps with assumptions:

  • Only managed server proxy apps are added to the manager. Unmanaged apps using #339 will not be added to manager as they cannot be controlled via JupyterLab frontend.
  • Once the simpervisor manages to start the proxy server app successfully, the metadata along with simpervisor process object is added to singleton manager object.
  • The manager is instantiated during loading of extension in __init__.py and added to ServerApp as a traitlet. This is inspired from the jupyter_lsp project. This singleton object is passed to API handlers and SupervisedProxyHandler.
  • If the user wishes to terminate a running server proxy app, the API call will effectively send a SIGTERM signal to running server proxy app by calling terminate method on simpervisor process.
  • In ensure_process method, we are verifying the state of the process using proc.running. When the running server proxy app is either terminated via JupyterLab frontend or if user terminates the app by sending a signal to PID manually proc.running will be evaluated to False. In these cases, we would like to ideally restart the process again and hence, we remove proc object from internal state and restart the process. _restart_process_if_needed will make sure simpervisor will restart the process in other cases.

Frontend Manager for the frontend follows a similar logic used to implement terminals/kernels sessions in JupyterLab.

  • Frontend manager makes polling to API at a configurable intervals to get the list of running server proxy apps and add them to running sessions in "Server Proxy Apps" section. jsp_settings
  • By hovering over an active server proxy app, metadata of the process (command, port, managed) can be viewed.
  • User can shutdown a given proxy app or all proxy apps using shutdown option in the running sessions. Proxy apps that are shutdown can be restarted afresh from the launcher.

Tests

  • API tests are included to check success and failure cases
  • Robot tests are included that shows running servers and shutting down a running server (I dont have a lot of experience robot framework and I would appreciate any help if they can be improved further).

Docs

Note about the Running Server Proxy Apps is included in GUI launchers in docs

Illustration ezgif-3-e8dcd463c7 or all server proxy apps which will terminate by sending SIGTERM as explained above.

Other changes

  • servers-info endpoint is moved from server-proxy/servers-info to server-proxy/api/servers-info to be consistent
  • jupyter_server is using root / and jupyterlab_server is using /lab/ directory for API endpoints. So, all API end points are moved to /server-proxy/ sub directory to avoid future incompatibility issues
  • An OpenAPI spec file is included in the repo to keep track of APIs and generate static docs
  • StaticFileHandler is used for IconHandler to simplify code

This extension can be ported to Notebook 7 as well with minimum changes.

This PR partially addresses #343, #264, #213, #188 (EDIT: marked as fixed, motivated below)

@consideRatio @ryanlovett @yuvipanda @bollwyvl Feel free to review and let me know your feedback!!

Edits by Erik

  • Fixes #188 The UI and API allows for management to terminate a proxied process

  • Fixes #213

    In order to free up resources it would be good to be able to stop the proxied service, then at some later time, decide to start it up again by clicking on the menu item in Jupyter.

    I believe this PR enables that.

  • Fixes #264

    I would like a feature to easily restart a service.

    I think this does the trick as well

  • Fixes #343 I understand that this PR provides an API to look this up.

mahendrapaipuri avatar Apr 12 '23 13:04 mahendrapaipuri

This is a reasonable starting point, but would benefit from aligning with the configuration and async patterns used in the upstream jupyter_server, as well as siblings such as jupyterlab_server and jupyter_lsp.

I'd say a lack of any python tests for the new features is probably a hard blocker to this getting merged, but really, with the more complex lab stuff, this would need to be tested by robot tests as well to be seriously considered for merging.

As noted, in general when adding a new REST APIs, starting with the schema and generating the python and TS types is a really good way to keep the system manageable, over time, and whether built with OpenAPI or not, can make the documentation process also much smoother.

bollwyvl avatar Apr 16 '23 14:04 bollwyvl

@consideRatio @bollwyvl Thanks for the feedback. I will work on the changes. Yes, I agree that we need to add tests for these features and I am working on them already. Next week I will be at KubeCon so, I might not find lot of time to work on this.

Are you aiming to make this extension compatible with Notebook 7? Currently the frontend extension fails as it is loading JupyterLab specific components. Straightforward solution is to load UI components conditionally, i.e., making ILauncher optional. Adding an optional Notebook related component (that add servers to Notebook launcher) to the extension should make it work for both Lab and Notebook interfaces. @bollwyvl Is it the strategy being followed now? I could not find a lot of info about extensions in new Notebook 7?

mahendrapaipuri avatar Apr 16 '23 18:04 mahendrapaipuri

@consideRatio @bollwyvl @manics I have update the PR with most of the comments addressed. Please check the updated PR description for more details.

Update: Cleaned up my mess and git history is clean now. Sorry about that!!

mahendrapaipuri avatar Apr 28 '23 09:04 mahendrapaipuri

@consideRatio I have reworked this PR based on the comments and added tests. Please check the updated PR description when you got time.

mahendrapaipuri avatar May 23 '23 07:05 mahendrapaipuri

This is amazing work @mahendrapaipuri!!!

I'm truly sorry we've not been able to review your work further yet @mahendrapaipuri. I'll do a small push now but I can't review this myself to completion.

As this PR is very large, avoid force pushing to the PR branch to help reviewers keep track of changes since last review better. I'll go through a few comments and link to the resolving commit to help other reviewers. I'd like to get the open review comments down to a minimum to reduce complexity of further review.

consideRatio avatar Sep 19 '23 10:09 consideRatio

Thanks a lot @consideRatio for reviewing the changes. It is totally fine, we all are busy with a lot of stuff and I understand things can take time.

I guess there are still few small things to address and I will do it next week. Cheers!!

mahendrapaipuri avatar Sep 20 '23 09:09 mahendrapaipuri

This adds great additional features to jupyter-server-proxy. I am looking forward to see it merged.

jhgoebbert avatar Aug 18 '24 11:08 jhgoebbert