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

nasa and hhmi hubs: update QGIS image #3797

Merged
merged 1 commit into from
Mar 15, 2024

Conversation

consideRatio
Copy link
Contributor

@consideRatio consideRatio commented Mar 14, 2024

Bumps to the image built via 2i2c-org/nasa-qgis-image#10.

  • Fixes https://github.com/2i2c-org/meta/issues/931

  • Removes a workaround to set singleuser.cmd: null
    This can be done as the docker image, based on quay.io/jupyter/minimal-notebook, since 2024-01-22 declares an ENTRYPOINT that includes executing the start.sh script.

    A Dockerfile's ENTRYPOINT / CMD can be overridden by specifying command / args in a k8s pod's container specification. The z2jh charts singleuser.cmd maps to KubeSpawner.cmd, and that configuration together with KubeSpawner.args is combined to set k8s pod's container args. This means KubeSpawner doesn't touch ENTRYPOINT aka. command in k8s specifications, making the start.sh script still be executed, while when it was set in jupyter/docker-stacks images CMD before 2024-01-22 it would get overridden.

Copy link

github-actions bot commented Mar 14, 2024

Merging this PR will trigger the following deployment actions.

Support and Staging deployments

Cloud Provider Cluster Name Upgrade Support? Reason for Support Redeploy Upgrade Staging? Reason for Staging Redeploy
gcp hhmi No Yes Following helm chart values files were modified: common.values.yaml
aws nasa-esdis No Yes Following helm chart values files were modified: common.values.yaml
aws nasa-veda No Yes Following helm chart values files were modified: common.values.yaml
aws nasa-ghg No Yes Following helm chart values files were modified: common.values.yaml

Production deployments

Cloud Provider Cluster Name Hub Name Reason for Redeploy
gcp hhmi prod Following helm chart values files were modified: common.values.yaml
aws nasa-esdis prod Following helm chart values files were modified: common.values.yaml
aws nasa-veda prod Following helm chart values files were modified: common.values.yaml
aws nasa-ghg prod Following helm chart values files were modified: common.values.yaml

@consideRatio consideRatio force-pushed the pr/update-qgis-images branch from 70e49ed to 43b73eb Compare March 14, 2024 10:25
@yuvipanda
Copy link
Member

@consideRatio can you help me understand why cmd: null is not needed anymore? It's primarily set for #3434, and that particular image requires start.sh to run (due to https://github.com/2i2c-org/nasa-qgis-image/blob/babb0fa15dfa5149016a019be595dffc483b08f8/Dockerfile#L14).

@yuvipanda
Copy link
Member

If this is based on jupyter/docker-stacks#2087 (comment), my understanding is that that shouldn't affect us here because Dockerfile's ENTRYPOINT is actually Kubernetes' cmd and Dockerfile's CMD is actually Kubernetes args.

@consideRatio
Copy link
Contributor Author

Hehe yes but what is kubespawners cmd? :) kubespawners cmd and kubespawners args combines into setting k8s args on the pod i think / from mobile

@consideRatio consideRatio force-pushed the pr/update-qgis-images branch from 43b73eb to 07e22d0 Compare March 14, 2024 21:52
@consideRatio consideRatio marked this pull request as ready for review March 14, 2024 22:04
@consideRatio consideRatio requested a review from a team as a code owner March 14, 2024 22:04
@consideRatio
Copy link
Contributor Author

I've updated the PR description to clarify this further @yuvipanda, and I've also updated the image tags. I consider this PR ready to be merged from a technical standpoint, but we are interfering with the communities user environment which we have no procedure around.

Do you think its OK to bump these images @yuvipanda?

@consideRatio
Copy link
Contributor Author

consideRatio commented Mar 14, 2024

Verified my understanding that kubespawner cmd maps to k8s args - k8s command isn't set on the user server's container when singleuser.cmd is the default of jupyterhub-singleuser. So this means that the dockerfile's ENTRYPOINT will be allowed to combine with the k8s specified args (overriding the Dockerfile's CMD).

kubectl get pod -n redacted jupyter-redacted -o yaml | grep -E -A5 "command|args"

  - args:
    - jupyterhub-singleuser
    env:
    - name: CPU_GUARANTEE
      value: "0.05"
    - name: JPY_API_TOKEN
--
  - command:
    - sh
    - -c
    - id && chown 1000:1000 /home/jovyan /home/jovyan/shared && ls -lhd /home/jovyan
    image: busybox:1.36.1
    imagePullPolicy: IfNotPresent
--
  - command:
    - iptables
    - --append
    - OUTPUT
    - --protocol
    - tcp

@yuvipanda
Copy link
Member

Ah, somewhere somehow my understanding needs to be rethought, but we don't need to wait for that to merge this. I tested this on staging and it works. Thanks for thinking this through, @consideRatio.

@yuvipanda yuvipanda merged commit a4a9569 into 2i2c-org:master Mar 15, 2024
11 checks passed
Copy link

🎉🎉🎉🎉

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

@consideRatio
Copy link
Contributor Author

Thank you @yuvipanda!!

I checked that repo2docker based images also doesn't have an issue with setting singleuser.cmd which led to #3807.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done 🎉
Development

Successfully merging this pull request may close these issues.

2 participants