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

fix docker volumn mount + notebook dir for admin #88

Merged
merged 3 commits into from
Sep 13, 2024
Merged

fix docker volumn mount + notebook dir for admin #88

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f24d9d4
fix docker volumn mount + notebook dir for admin
Tianhao-Gu Sep 13, 2024
4a99070
add a comment
Tianhao-Gu Sep 13, 2024
5dc6cdd
using root as admin notebook dir
Tianhao-Gu Sep 13, 2024
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
12 changes: 6 additions & 6 deletions docker-compose.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ services:
- POSTGRES_DB=hive
- POSTGRES_URL=postgres:5432
volumes:
- ./cdr/cdm/jupyter:/cdm_shared_workspace
- ./cdr/cdm/jupyter/cdm_shared_workspace:/cdm_shared_workspace

spark-worker-1:
# The latest image from cdm-jupyterhub that includes spark standalone mode
Expand All @@ -68,7 +68,7 @@ services:
- POSTGRES_DB=hive
- POSTGRES_URL=postgres:5432
volumes:
- ./cdr/cdm/jupyter:/cdm_shared_workspace
- ./cdr/cdm/jupyter/cdm_shared_workspace:/cdm_shared_workspace

spark-worker-2:
# The latest image from cdm-jupyterhub that includes spark standalone mode
Expand All @@ -90,7 +90,7 @@ services:
- POSTGRES_DB=hive
- POSTGRES_URL=postgres:5432
volumes:
- ./cdr/cdm/jupyter:/cdm_shared_workspace
- ./cdr/cdm/jupyter/cdm_shared_workspace:/cdm_shared_workspace

minio:
image: minio/minio
Expand Down Expand Up @@ -150,7 +150,7 @@ services:
- POSTGRES_URL=postgres:5432
- USAGE_MODE=dev # Enabling dev mode grants full access to MinIO and additional privileges for services like Hive, such as the ability to create tables as defined in the scripts/setup.sh.
volumes:
- ./cdr/cdm/jupyter:/cdm_shared_workspace
- ./cdr/cdm/jupyter/cdm_shared_workspace:/cdm_shared_workspace

user-jupyterlab:
build:
Expand Down Expand Up @@ -179,7 +179,7 @@ services:
- POSTGRES_DB=hive
- POSTGRES_URL=postgres:5432
volumes:
- ./cdr/cdm/jupyter/user_shared_workspace:/cdm_shared_workspace/user_shared_workspace
- ./cdr/cdm/jupyter/cdm_shared_workspace/user_shared_workspace:/cdm_shared_workspace/user_shared_workspace

cdm_jupyterhub:
build:
Expand Down Expand Up @@ -211,7 +211,7 @@ services:
- POSTGRES_URL=postgres:5432
- JUPYTERHUB_ADMIN_PASSWORD=testpassword123
volumes:
- ./cdr/cdm/jupyter:/cdm_shared_workspace
- ./cdr/cdm/jupyter/cdm_shared_workspace:/cdm_shared_workspace # Legacy data volume from JupyterLab
- ./cdr/cdm/jupyter/jupyterhub_secrets:/jupyterhub_secrets
- ./cdr/cdm/jupyter/jupyterhub/users_home:/jupyterhub/users_home

Expand Down
3 changes: 2 additions & 1 deletion src/jupyterhub_config/custom_spawner.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,8 @@ def _configure_notebook_dir(self, username: str, user_dir: Path):
"""
if self.user.admin:
self.log.info(f'Admin user detected: {username}. Setting up admin workspace.')
self.notebook_dir = '/cdm_shared_workspace'
# global users' home directory
self.notebook_dir = str(user_dir.parent)
Copy link
Member

Choose a reason for hiding this comment

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

So basically admin notebooks are all shared and live in /home and user notebooks live in /home/{user}? And users can read /home/{user} but not home?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

hmm. Maybe we should use /home/{user} for all? Now I am thinking, admin user actually don't have access to user's home dir. I was thinking only a handful person (tech team or maybe just spark_user) should be admin, and assign groups for permissions. Admin user won't create notebooks.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think str('/') makes more sense for admin user.

Copy link
Member

Choose a reason for hiding this comment

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

or /root

Copy link
Member

Choose a reason for hiding this comment

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

or /admin

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

👍 just changed it to '/' for now. I think most majority users won't be admin.

Copy link
Member

@MrCreosote MrCreosote Sep 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

That means any notebooks they create won't be persisted over a container recreation, I assume

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yea yea. I think they (or most likely just us) just need to remember to create notebook in the jupyterhub/users_home dir.

else:
self.log.info(f'Non-admin user detected: {username}. Setting up user-specific workspace.')
self.notebook_dir = str(user_dir)
10 changes: 6 additions & 4 deletions test/src/jupyterhub_config/custom_spawner_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -446,17 +446,19 @@ def test_configure_environment_missing_pythonpath(spawner):
assert f"{user_env_dir}/lib/python3.11/site-packages" == spawner.environment['PYTHONPATH']


@pytest.mark.parametrize("is_admin, expected_dir", [
(True, '/cdm_shared_workspace'), # Admin user case
(False, 'to_be_defined_in_the_test') # Non-admin user case
@pytest.mark.parametrize("is_admin", [
True, # Admin user case
False # Non-admin user case
])
def test_configure_notebook_dir(is_admin, expected_dir, spawner, caplog):
def test_configure_notebook_dir(is_admin, spawner, caplog):
spawner.user.admin = is_admin
username = 'testuser'
Copy link
Member

Choose a reason for hiding this comment

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

This would probably be simpler as just a loop at this point, like

for b in [True, False]:
    spawner.user.admin = b
    ...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

👍

with tempfile.TemporaryDirectory() as temp_dir:
user_dir = Path(temp_dir) / 'testuser'
if not is_admin:
expected_dir = str(user_dir)
else:
expected_dir = str(user_dir.parent)

with caplog.at_level(logging.INFO):
spawner._configure_notebook_dir(username, user_dir)
Expand Down
Loading