search

jupyterhub / batchspawner

Custom Spawner for Jupyterhub to start servers in batch scheduled systems
BSD 3-Clause "New" or "Revised" License
190 stars 134 forks source link

Support named servers #167

Closed rcthomas closed 5 years ago

rcthomas commented 5 years ago

Since v0.8, JupyterHub has supported "named servers." If c.JupyterHub.allow_named_servers=True in the hub config, servers can have names assigned to them. If this setting is False then there is only one server with the name '' (empty string).

For BatchSpawner to work with named servers, the port selected by batchspawner-singleuser must be routed to the right spawner. Right now the port is attached to user.spawner but this is really just an alias for user.spawners['']. Without this fix the port value lands on the empty-string spawner and that may not be what the user wanted to happen at all.

To tell the hub which spawner to attach the port information to, this PR has batchspawner-singleuser read the value of the environment variable JUPYTERHUB_SERVER_NAME and send it back to the hub as part of the API call. The API is modified to consume this value and use it to attach the port to the right spawner.

rcthomas commented 5 years ago

With this fix, BatchSpawner appears to work at our center, including with WrapSpawner and named servers. It does not look like there is any issue with WrapSpawner.

cmd-ntrf commented 5 years ago

You could identify the spawner based on the token value instead of sending the server name through the API.

This is what I did here : https://github.com/cmd-ntrf/batchspawner/commit/5a5de660214de4924b81cca8e115e5a0540be992

I am not sure why it was never merged upstream, I might have forgotten to do a PR or it was lost in the merges.

rcthomas commented 5 years ago

Ah that's even better. I'll rework.

rcthomas commented 5 years ago

Modified to identify correct spawner based on API token. Decided to set None as the default spawner value that way if there is no matching token at all then the attempt to set the port fails. We could replace this with a more meaningful failure but I thought at least if we are checking the API token all the time, we should make it fail if there is no match.

mbmilligan commented 5 years ago

Looks good to me, merging.

Sorry @cmd-ntrf if I missed a PR from you to fix this earlier!