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

enable passing server options for notebook server launch #68

Merged
merged 9 commits into from
Oct 18, 2018
Merged

Conversation

rokroskar
Copy link
Member

@rokroskar rokroskar commented Oct 16, 2018

This PR allows the notebooks service setup to include default server options -- and to override them upon server launch. The options are set in values.yaml and written into a file in the container via a configMap.

The options can be queried by a UI client using GET <service_prefix>/<namespace>/<project>/<commit-sha>/server_options -- the returned JSON can be used to render a template.

The client should return only the option and the value, e.g. if the JSON server from server_options is

{
  "serverOptions":
  { 
    "resources":
      { 
        "cpu_limit":
         {
          "default": 1,
          "type": int,
          "range": [1,4]
          }
      }
  }
}

The client should use

{
  "serverOptions":
    {
      "resources":
      {
         "cpu_limit": "1"
      }
    }
}

as the json data in the POST request to spawn the server.


closes #66
closes #65
addresses SwissDataScienceCenter/renku#443

@rokroskar rokroskar requested review from ableuler and leafty and removed request for ableuler October 16, 2018 07:06
@rokroskar rokroskar changed the title enable passing server options for notebook server launch [WIP] enable passing server options for notebook server launch Oct 16, 2018
@@ -2,4 +2,4 @@ apiVersion: v1
appVersion: '1.0'
description: A Helm chart for the Renku Notebooks service
name: renku-notebooks
version: 0.2.0
version: 0.0-unclean-191b684
Copy link
Member

Choose a reason for hiding this comment

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

🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes I know, I'll clean all this up when I'm done running it on my minikube :)

Copy link
Member

@leafty leafty left a comment

Choose a reason for hiding this comment

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

looks good

ports:
- name: http
containerPort: 8000
protocol: TCP
volumeMounts:
- name: server-options
mountPath: /etc/renku-notebooks/server_options.json
Copy link
Member

Choose a reason for hiding this comment

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

@@ -1,4 +1,4 @@
FROM jupyterhub/k8s-hub:0.7.0
FROM jupyterhub/k8s-hub:ae44b17
Copy link
Member

Choose a reason for hiding this comment

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

huh?

jupyterhub/spawners.py Show resolved Hide resolved
src/notebooks_service.py Show resolved Hide resolved
## Configuration for the jupyterhub service
jupyterhub:
rbac:
enabled: true
hub:
image:
name: renku/jupyterhub-k8s
tag: '0.2.0'
tag: '0.0-unclean-191b684'
Copy link
Member

Choose a reason for hiding this comment

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

🙈

@@ -140,14 +165,14 @@ jupyterhub:
singleuser:
image:
name: renku/singleuser
tag: '0.2.0'
tag: '0.0-unclean-191b684'
Copy link
Member

Choose a reason for hiding this comment

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

🙊

storage:
type: none
defaultUrl: /lab
singleuser-r:
image:
name: renku/singleuser-r
tag: 'latest'
tag: '0.0-unclean-191b684'
Copy link
Member

Choose a reason for hiding this comment

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

🙉

Copy link
Contributor

@ableuler ableuler left a comment

Choose a reason for hiding this comment

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

Looking good so far. Can you please make sure that the default is always part of the available options for the enum case? This is currently not the case for the memory requirements.

mem_limit:
displayName: Memory
type: enum
default: 1G
Copy link
Member

Choose a reason for hiding this comment

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

default 2G

Copy link
Member

@leafty leafty left a comment

Choose a reason for hiding this comment

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

NB: From the code of kubespawner, 0 can be used for no limit

displayName: Number of CPUs
type: float
default: 1.0
range: [0.1, 4.0]
Copy link
Member

Choose a reason for hiding this comment

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

only use discrete options

Copy link
Member Author

Choose a reason for hiding this comment

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

why? CPUs requests are fractional

Copy link
Member

Choose a reason for hiding this comment

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

a. are you going to use it?
b. with options: [0, 1, 2, 4, 8] you can have 0 = no limits
c. you can include 0.5 anyway

Copy link
Member Author

@rokroskar rokroskar Oct 17, 2018

Choose a reason for hiding this comment

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

this is a limit, not a request -- so I think actually integer values are probably fine, so I propose we do range: [1,X] (I just pushed this change in de26aaa)

Copy link
Member

Choose a reason for hiding this comment

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

0 is valid :p

Copy link
Member Author

Choose a reason for hiding this comment

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

yes but we don't want people to specify 0. In any case, from what I've read the cpu limits are not strictly enforced, unlike the memory limits.

helm-chart/renku-notebooks/values.yaml Show resolved Hide resolved
@rokroskar
Copy link
Member Author

btw, I don't pass 0 as a gpu number -- if it is 0 then we don't ask for a gpu at all.

leafty
leafty previously approved these changes Oct 17, 2018

gpu = server_options['resources'].get('gpu', {})
if gpu:
self.extra_resource_guarantees = {
Copy link
Member

Choose a reason for hiding this comment

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

extra_resource_limits

@rokroskar rokroskar changed the title [WIP] enable passing server options for notebook server launch enable passing server options for notebook server launch Oct 17, 2018
@rokroskar rokroskar dismissed ableuler’s stale review October 18, 2018 07:36

this has been fixed

@rokroskar rokroskar merged commit b0c93bb into master Oct 18, 2018
@rokroskar rokroskar deleted the fix-66 branch October 18, 2018 07:37
rokroskar added a commit to SwissDataScienceCenter/renku that referenced this pull request Oct 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

use server options for launching jupyter servers include possibility to add GPU requirements to pod manifest
3 participants