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 Hub prototype] Binderhub UI demo #4119

Merged
merged 24 commits into from
May 30, 2024

Conversation

GeorgianaElena
Copy link
Member

@GeorgianaElena GeorgianaElena commented May 22, 2024

Related to #4086 and #4133

What happens in this PR:

  • disable hub's storage via jupyterhub.storage.type = none
  • swap the spawner with BinderSpawnerMixin and enable_auth on it
  • deploy binderhub as a jupyterhub externally managed service
  • enable the binderhub-service ingress and configure it so that we get a public domain for binderhub to run
    • this means we ca stop specifying a service. This is because services.url is meant to be added to the proxy at /services/:name and we don't need it because we intend to use the service's public url set by the ingress
  • todo continue the list

This comment was marked as resolved.

@consideRatio
Copy link
Member

consideRatio commented May 22, 2024

We were thinking about this stuff below, and how binderhub_config.py in the official binderhub chart includes some auth stuff that isn't in binderhub-service.

        - name: JUPYTERHUB_SERVICE_NAME
          value: binder
        - name: JUPYTERHUB_API_URL
          value: {{ (print (.Values.config.BinderHub.hub_url_local | default .Values.config.BinderHub.hub_url | trimSuffix "/") "/hub/api/") }}
        - name: JUPYTERHUB_BASE_URL
          value: {{ .Values.jupyterhub.hub.baseUrl | quote }}
        - name: JUPYTERHUB_CLIENT_ID
          value: {{ .Values.jupyterhub.hub.services.binder.oauth_client_id | quote }}
        - name: JUPYTERHUB_OAUTH_CALLBACK_URL
          value: {{ .Values.jupyterhub.hub.services.binder.oauth_redirect_uri | quote }}

@GeorgianaElena
Copy link
Member Author

There's almost a functional binderhub running at https://binderhub-ui-demo.2i2c.cloud. Last bit that's missing is launching that's currently failing due to a 403 related to scopes. Also last two commits were hacks to quickly fix a couple of issues I had with registry credentials. They weren't properly investigated yet.

@consideRatio
Copy link
Member

Last bit that's missing is launching that's currently failing due to a 403 related to scopes.

Looking at this now - we haven't provided permissions to the jupyterhub registered service "binder" to launch servers.

This was only required for configuring the official binderhub chart
which overrides singleuser.cmd, but we haven't done that in the first
place so we don't need to re-set it to jupyterhub-singleuser.
@consideRatio
Copy link
Member

consideRatio commented May 28, 2024

@GeorgianaElena amazing effort into this!!

I got a launch to work all the way with small final tweaks, the tweaks where:

  • provide permissions for the binder jupyterhub service and its generated API token to use the REST API to spawn servers (relevant for launch) and inspect users (relevant for auth).
  • removed some volume mounting of home directories setup via basehub we can't do now without hub specific storage
  • (refactoring, not required) removed re-declaration of jupyterhub-singleuser which is the default of z2jh and unmodified by binderhub-service (but not by official binderhub chart!)

Related to jupyterhub-singleuser, I raised a question about that in jupyterhub/binderhub#1854, as that is a requirement on the environment not part of mybinder.org launches for example.

@GeorgianaElena
Copy link
Member Author

Yuhooooo 🚀

Comment on lines +87 to +94
extraConfig:
# FIXME: set KubernetesBuildExecutor.push_secret again
# without this for some reason the build pods
# search after the wrong secret name (i.e. the default name)
# set by binderhub in KubernetesBuildExecutor.push_secret
01-binderhub-service-set-push-secret: |
import os
c.KubernetesBuildExecutor.push_secret = os.environ["PUSH_SECRET_NAME"]
Copy link
Member Author

Choose a reason for hiding this comment

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

So I have no idea what happened here. I tried investigated and started by removing this hack and checked again and this time it worked. Maybe there's something relevant that didn't get properly cleared during the redeploy?

Anyway, I will drop the commit that added this, but leaving this here for posterity in case there is indeed something going on:

This is what the build pod showed before this hack, yesterday

Warning  FailedMount  6m4s (x114 over 3h41m)  kubelet  MountVolume.SetUp failed for volume "docker-config" : secret "binder-build-docker-config" not found
Volumes:
  docker-socket:
    Type:          HostPath (bare host directory volume)
    Path:          /var/run/docker.sock
    HostPathType:  Socket
  docker-config:
    Type:        Secret (a volume populated by a Secret)
    SecretName:  binder-build-docker-config
    Optional:    false
  kube-api-access-687gt:
    Type:                    Projected (a volume that contains injected data from multiple sources)
    TokenExpirationSeconds:  3607
    ConfigMapName:           kube-root-ca.crt
    ConfigMapOptional:       <nil>
    DownwardAPI:             true

Copy link
Member

@consideRatio consideRatio May 28, 2024

Choose a reason for hiding this comment

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

Every time I work with binderhub, the push_secret config is a big confusion =/

TL;DR - I was confused, and remain confused why you observed that.

Here are reasons for why its confusing observed in the last ~5 minutes looking:

  1. push_secret should be configured to a k8s secret name having a config.json key providing a docker configuration including credentials to work against a registry (its not the typical k8s imagePullSecret or similar, its a docker client config)
  2. binder-build-docker-config is a default value for push_secret, but its also hardcoded i the official BinderHub helm chart deployment, but in binderhub-service deployment it isn't hardcoded - instead push_secret is set by a extraConfig entry based on an env variable etc - we shouldn't touch push_secret config as users of binderhub-service I think.
  3. In binderhub-service chart, we configure credentials that goes into the k8s Secret via chart config buildPodsRegistryCredentials, but in the official chart its under config.BinderHub.buildDockerConfig which is confusing because it isn't a traitlet for the Python class BinderHub as expected if listed there.
  4. The binderhub-service deployment resource isn't automatically restarting if the credentials set via buildPodsRegistryCredentials is updated - it only gets restarted if there is something in the pod spec that changes, such as an annotation providing a hash of the secret. I'll fix this. EDIT: nevermind what I wrote doesn't make sense I realize.

domain: hub.binderhub-ui-demo-demo.2i2c.cloud
helm_chart: basehub
helm_chart_values_files:
- basehub-common.values.yaml
Copy link
Member

Choose a reason for hiding this comment

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

This just enables nfs things we won't use, should be removed I think.

@yuvipanda
Copy link
Member

fwiw, it looks to me that the deployment satisfies all the criteria (except the about message perhaps?) specified in #4133. If that's right, I'd suggest we review and get this merged, and generate follow-up tasks from here instead of keeping this PR open until it is 'all' sorted.

@consideRatio
Copy link
Member

I'll go for a merge to flush what we have to ensure we get the terraform state changes represented - we haven't provided announcement, and there are things to refine and think about before the pattern of deploying a configuration like this is replicated even though that hasn't yet been specified.

@consideRatio consideRatio marked this pull request as ready for review May 30, 2024 19:00
@consideRatio consideRatio merged commit c84b389 into 2i2c-org:main May 30, 2024
37 checks passed
Copy link

🎉🎉🎉🎉

Monitor the deployment of the hubs here 👉 https://github.com/2i2c-org/infrastructure/actions/runs/9307796142

This pull request was closed.
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.

3 participants