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

fix: Mount /dev/shm to the correct container. #1984

Merged
merged 2 commits into from
Sep 19, 2024

Conversation

aledegano
Copy link
Contributor

@aledegano aledegano commented Sep 18, 2024

Currently container #1 is oauth-proxy while JupyterServer, is #0.

Additionally use half of the memory requested by the pod instead of half of the storage.

Since we rely on memory there is no need to tie the increased /dev/shm to requesting storage in the session.

NOTE: This system of patching the manifest is very brittle and the bug being fixed by this commit might appear again as soon as the containers sorting changes. We should think about a different way of interacting with the manifests of the user-sessions.

/deploy renku=leafty/fix-renku-jupyterserver

@RenkuBot
Copy link
Contributor

You can access the deployment of this PR at https://renku-ci-nb-1984.dev.renku.ch

@aledegano aledegano marked this pull request as ready for review September 18, 2024 08:52
@aledegano aledegano requested a review from a team as a code owner September 18, 2024 08:52
Copy link
Member

@olevski olevski left a comment

Choose a reason for hiding this comment

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

Thanks Alessandro. One suggestion to make sure that things are ok if memory is None.

renku_notebooks/api/amalthea_patches/general.py Outdated Show resolved Hide resolved
@aledegano
Copy link
Contributor Author

Thanks Alessandro. One suggestion to make sure that things are ok if memory is None.

Thanks, I've included your change.
I think we always set the memory for sessions, but it doesn't hurt to check!

@olevski
Copy link
Member

olevski commented Sep 18, 2024

NOTE: This system of patching the manifest is very brittle and the bug being fixed by this commit might appear again as soon as the containers sorting changes. We should think about a different way of interacting with the manifests of the user-sessions.

I absolutely agree. The new version of amalthea does not have patches like this at all. So when we retire the old version this code will just be thrown out.

Currently container #1 is `oauth-proxy` while JupyterServer,
is #0.

Additionally use half of the memory requested by the pod
instead of half of the storage.

Since we rely on memory there is no need to tie the increased
/dev/shm to requesting storage in the session.

NOTE: This system of patching the manifest is very brittle and
the bug being fixed by this commit might appear again as soon
as the containers sorting changes. We should think about a different
way of interacting with the manifests of the user-sessions.
@aledegano aledegano force-pushed the fix_shm_mount_container branch from 08bab41 to 24740fc Compare September 18, 2024 09:32
@aledegano aledegano merged commit 8b47163 into master Sep 19, 2024
37 of 39 checks passed
@aledegano aledegano deleted the fix_shm_mount_container branch September 19, 2024 09:09
olevski pushed a commit to SwissDataScienceCenter/renku-data-services that referenced this pull request Dec 10, 2024
olevski added a commit to SwissDataScienceCenter/renku-data-services that referenced this pull request Dec 10, 2024
Migrating
SwissDataScienceCenter/renku-notebooks#1984

Co-authored-by: Alessandro Degano <alessandro.degano@epfl.ch>
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.

4 participants