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.

Sign up for GitHub

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

Stop using continuous prePuller #3313

Merged
merged 2 commits into from
Oct 31, 2023
Merged

Stop using continuous prePuller #3313

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d403b4c
Stop using continuous prePuller
consideRatio Oct 24, 2023
a23c0da
Apply suggestions from code review
consideRatio Oct 31, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 2 additions & 6 deletions config/clusters/2i2c/climatematch.values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,7 @@
jupyterhub:
# pre-puller is necessary as the image is pretty big, and
# pulling during first user spawn might cause timeouts and poor user
# experience. Also helps with node pre-warming. This works ok here
# because they have a dedicated nodepool.
prePuller:
continuous:
enabled: true
# hook prePuller shouldn't be enabled when configuring images in any other
# way than singleuser.image
hook:
enabled: true
singleuser:
Expand Down
7 changes: 2 additions & 5 deletions config/clusters/callysto/common.values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,9 @@ nfs:
# Name of Google Filestore share
baseShareName: /homes/
jupyterhub:
# pre-puller is necessary as the image is pretty big, and
# pulling during first user spawn might cause timeouts and poor user
# experience. Also helps with node pre-warming.
prePuller:
continuous:
enabled: true
# hook prePuller shouldn't be enabled when configuring images in any other
# way than singleuser.image
hook:
enabled: true
custom:
Expand Down
4 changes: 1 addition & 3 deletions config/clusters/catalystproject-latam/common.values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,7 @@ jupyterhub:
allowNamedServers: true
singleuser:
image:
# This image specification is likeley overridden via the configurator, so
# use of prePuller is probably going to pull this instead of the
# configured image and just slow users down.
# This image specification is likely overridden via the configurator.
#
# jupyter/scipy-notebook is maintained at: https://github.com/jupyter/docker-stacks
# tags can be viewed at: https://hub.docker.com/r/jupyter/scipy-notebook/tags
Expand Down
4 changes: 2 additions & 2 deletions config/clusters/cloudbank/demo.values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@ jupyterhub:
- hosts: [demo.cloudbank.2i2c.cloud]
secretName: https-auto-tls
prePuller:
continuous:
enabled: true
# hook prePuller shouldn't be enabled when configuring images in any other
# way than singleuser.image
hook:
enabled: true
singleuser:
Expand Down
4 changes: 2 additions & 2 deletions config/clusters/cloudbank/humboldt.values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@ jupyterhub:
- hosts: [humboldt.cloudbank.2i2c.cloud]
secretName: https-auto-tls
prePuller:
continuous:
enabled: true
# hook prePuller shouldn't be enabled when configuring images in any other
# way than singleuser.image
hook:
enabled: true
singleuser:
Expand Down
4 changes: 2 additions & 2 deletions config/clusters/cloudbank/sjsu.values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@ jupyterhub:
- hosts: [sjsu.cloudbank.2i2c.cloud]
secretName: https-auto-tls
prePuller:
continuous:
enabled: true
# hook prePuller shouldn't be enabled when configuring images in any other
# way than singleuser.image
hook:
enabled: true
singleuser:
Expand Down
3 changes: 0 additions & 3 deletions config/clusters/gridsst/common.values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,6 @@ basehub:
serverIP: fs-05f68d7e096d7cf16.efs.us-west-2.amazonaws.com
baseShareName: /
jupyterhub:
prePuller:
continuous:
enabled: true
custom:
2i2c:
add_staff_user_ids_to_admin_users: true
Expand Down
4 changes: 2 additions & 2 deletions config/clusters/qcl/common.values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@ nfs:
baseShareName: /homes/
jupyterhub:
prePuller:
continuous:
enabled: true
# hook prePuller shouldn't be enabled when configuring images in any other
# way than singleuser.image
hook:
enabled: true
custom:
Expand Down
7 changes: 2 additions & 5 deletions config/clusters/utoronto/common.values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,9 @@ jupyterhub:
scheduling:
userScheduler:
enabled: true
# pre-puller is necessary as the image is pretty big, and
# pulling during first user spawn might cause timeouts and poor user
# experience. Also helps with node pre-warming.
prePuller:
continuous:
enabled: true
# hook prePuller shouldn't be enabled when configuring images in any other
# way than singleuser.image
hook:
enabled: true
custom:
Expand Down
25 changes: 5 additions & 20 deletions docs/howto/features/ephemeral.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,34 +107,19 @@ jupyterhub:
```


## Pre-pulled images
## Image configuration in chart

We want *consistently* faster startups wherever possible, as inconsistent start
times is one of the big issues with folks using mybinder.org for events and
workshops. So we enable the [pre-puller](https://z2jh.jupyter.org/en/latest/administrator/optimization.html#pulling-images-before-users-arrive)
functionality to make startups faster and more consistent.

This requires the user image is also set in config (and not via the JupyterHub
configurator). But `tmpauthenticator` doesn't support admin accounts anyway,
so this is fine.
The image needs to be specified in the chart directly and not via the JupyterHub
configurator because with `tmpauthenticator` we can't distinguish admin users to
have such rights without providing it to every user.

```yaml
jupyterhub:
singleuser:
# image could also be configured via singleuser.profileList configuration
image:
name: <image-name>
tag: <tag>

prePuller:
# Startup performance is important for this event, and so we use
# pre-puller to make sure the images are already present on the
# nodes. This means image *must* be set in config, and not the configurator.
# tmpauthenticator doesn't support admin access anyway, so images
# must be set in config regardless.
hook:
enabled: true
continuous:
enabled: true
```

## Disabling home page customizations
Expand Down
27 changes: 26 additions & 1 deletion helm-charts/basehub/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,9 +143,34 @@ jupyterhub:
memory: 64Mi
limits:
memory: 1G
# prePuller is about pulling a one or more images identified via chart
# configuration, including singleuser.image, singleuser.profileList entries
# with a dedicated image, but not profileList entries with images' specified
# via profile_options.
prePuller:
# continuous prePuller leads to the creation of a DaemonSet that starts a
# pod on each node to pull images.
#
# It is disabled as its only relevant for nodes started before user pods
# gets scheduled on them, in other cases it could delay startup and isn't
# expected to reduce startup times.
#
Comment on lines +146 to +157
Copy link
Member

Choose a reason for hiding this comment

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

@consideRatio, what if we add these comments as a descriptions in the schema file?

I figure, the schema is where we currently "document" basehub defaults and having these in there kind of makes sense? Maybe in the future we can use that to generate some sort of API-like docs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe in the future we can use that to generate some sort of API-like docs?

I agree on letting basehub generate config reference docs based on descriptions for basehub added config in values.schema.yaml like done by z2jh!

I favor the approach of inlining a comment next to the changed default value in this case though.

  1. All charts require a values.yaml file, so helm chart users likely look in them to learn about default values. If we separate remarks about default values into a schema file / generated docs, they may be less findable from the values.yaml file.
  2. We don't yet have generated docs from the basehub's values.schema.yaml
  3. If/when we generate docs from the schema, should the schema really redefine parts of the schema of dependency charts to provide a description that the default value is changed?

continuous:
enabled: false
# hook prePuller leads to the creation of a temporary DaemonSet and a pod
# awaiting pulling to complete before `helm upgrade` starts its main work.
#
# It is disabled as it adds notable complexity for a smaller benefit when
# correctly adopted. The added complexity includes:
#
# - risk of misconfiguration making image pulls not actually needed
# - risk of broken expectations and additional cognitive load
# - risk of causing significantly longer `helm upgrade` commands slowing
# down our CI system
# - ClusterRoleBinding resources are needed for the image-awaiter Pod
# involved, a resource that requires the highest k8s cluster permission
# otherwise possibly not needed to deploy basehub
#
hook:
enabled: false
proxy:
Expand Down Expand Up @@ -267,7 +292,7 @@ jupyterhub:
MappingKernelManager: *server_config_mapping_kernel_manager
NotebookApp: *server_config_server_app
BaseFileIdManager: *server_config_base_file_id_manager
startTimeout: 600 # 10 mins, because sometimes we have too many new nodes coming up together
startTimeout: 600 # 10 mins, node startup + image pulling makes it relevant
consideRatio marked this conversation as resolved.
Show resolved Hide resolved
defaultUrl: /tree
image:
name: jupyter/scipy-notebook
Expand Down