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

mount user's volume #94

Merged
merged 1 commit into from
Sep 23, 2024
Merged

mount user's volume #94

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
44 changes: 40 additions & 4 deletions src/jupyterhub_config/custom_docker_spawner.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,9 @@
# Configure the notebook directory based on whether the user is an admin
self._configure_notebook_dir(username, user_dir)

# Ensure the user's volume is correctly mounted in the container
self._ensure_user_volume()

Check warning on line 33 in src/jupyterhub_config/custom_docker_spawner.py

View check run for this annotation

Codecov / codecov/patch

src/jupyterhub_config/custom_docker_spawner.py#L33 

Added line #L33 was not covered by tests

return super().start()

def _ensure_user_directory(self, user_dir: Path, username: str):
Expand Down Expand Up @@ -101,10 +104,7 @@
self.environment['PYTHONSTARTUP'] = os.path.join(os.environ['JUPYTERHUB_CONFIG_DIR'], 'startup.py')
self.environment['JUPYTERHUB_USER'] = username

group_names = [group.name for group in self.user.groups]
self.log.info(f'User {self.user.name} groups: {group_names}')

if self.user.admin or self.RW_MINIO_GROUP in group_names:
if self._is_rw_minio_user():

Check warning on line 107 in src/jupyterhub_config/custom_docker_spawner.py

View check run for this annotation

Codecov / codecov/patch

src/jupyterhub_config/custom_docker_spawner.py#L107 

Added line #L107 was not covered by tests
self.log.info(f'MinIO read/write user detected: {self.user.name}. Setting up minio_rw credentials.')
self.environment['MINIO_ACCESS_KEY'] = self.environment['MINIO_RW_ACCESS_KEY']
self.environment['MINIO_SECRET_KEY'] = self.environment['MINIO_RW_SECRET_KEY']
Expand All @@ -131,3 +131,39 @@
else:
self.log.info(f'Non-admin user detected: {username}. Setting up user-specific workspace.')
self.notebook_dir = str(user_dir)

def _is_rw_minio_user(self):
"""
Check if the user is a read/write MinIO user.

Admin users and users in the minio_rw group are considered read/write MinIO users.
"""
group_names = [group.name for group in self.user.groups]
return self.user.admin or self.RW_MINIO_GROUP in group_names

Check warning on line 142 in src/jupyterhub_config/custom_docker_spawner.py

View check run for this annotation

Codecov / codecov/patch

src/jupyterhub_config/custom_docker_spawner.py#L141-L142 

Added lines #L141 - L142 were not covered by tests

def _ensure_user_volume(self):
"""
Ensure the user's volume is correctly mounted in the container.
"""

user_home_dir = Path(os.environ['JUPYTERHUB_USER_HOME'])
mount_base_dir = Path(os.environ['JUPYTERHUB_MOUNT_BASE_DIR'])
hub_secrets_dir = Path(os.environ['JUPYTERHUB_SECRETS_DIR'])

Check warning on line 151 in src/jupyterhub_config/custom_docker_spawner.py

View check run for this annotation

Codecov / codecov/patch

src/jupyterhub_config/custom_docker_spawner.py#L149-L151 

Added lines #L149 - L151 were not covered by tests
MrCreosote marked this conversation as resolved.
Show resolved Hide resolved

cdm_shared_dir = Path(os.environ['CDM_SHARED_DIR']) # Legacy data volume from JupyterLab
Copy link
Member

Choose a reason for hiding this comment

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

Should we just get rid of this? Seems like a good time to do so if we're switching infrastructure

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

hmmm I am thinking we will need to mount a shared dir for people to put shared notebooks. Also, I am planning to use admin account to move existing notebooks to user's new workspace. For the later, there are other ways.

Copy link
Member

Choose a reason for hiding this comment

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

So it's not really legacy then?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

hmmm yea. Maybe I will remove the comments or remove the mount once we are set stone of mounting volume.

Copy link
Member

Choose a reason for hiding this comment

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

👍

hive_metastore_dir = Path(os.environ['HIVE_METASTORE_DIR']) # within cdm_shared_dir

Check warning on line 154 in src/jupyterhub_config/custom_docker_spawner.py

View check run for this annotation

Codecov / codecov/patch

src/jupyterhub_config/custom_docker_spawner.py#L153-L154 

Added lines #L153 - L154 were not covered by tests

if self.user.admin:
self.log.info(f'Admin user detected: {self.user.name}. Setting up admin mount points.')
self.volumes.update({

Check warning on line 158 in src/jupyterhub_config/custom_docker_spawner.py

View check run for this annotation

Codecov / codecov/patch

src/jupyterhub_config/custom_docker_spawner.py#L156-L158 

Added lines #L156 - L158 were not covered by tests
f'{mount_base_dir}/{user_home_dir}': f'{user_home_dir}', # Global users home directory
f'{mount_base_dir}/{hub_secrets_dir}': f'{hub_secrets_dir}',
f'{mount_base_dir}/{cdm_shared_dir}': f'{cdm_shared_dir}', # Legacy data volume from JupyterLab
Comment on lines +159 to +161
Copy link
Member

Choose a reason for hiding this comment

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

Admin doesn't need the hive metastore dir?

Actually, didn't we say last time there shouldn't be a hive dir at all?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

hive is within cdm_shared_dir. So I didn't mount it here specifically.

Copy link
Member

Choose a reason for hiding this comment

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

But didn't we say it's not needed in a recent pr?

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 you are referring to postgres dir which is remote now so we don't need it. I think users need to read hive in order to read correctly.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

seems like when a namespace is created in postgres, an associated namespace.db file is created in the hive dir. So I am guessing it's needed. But we can remove it if we noticed it's no longer needed. I want to grant users broad privileges initially, then scale them back gradually based on our findings. That way we can move faster.

Copy link
Member

Choose a reason for hiding this comment

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

👍

})
else:
self.log.info(f'Non-admin user detected: {self.user.name}. Setting up user-specific mount points.')
access_mode = 'rw' if self._is_rw_minio_user() else 'ro'
self.volumes.update({

Check warning on line 166 in src/jupyterhub_config/custom_docker_spawner.py

View check run for this annotation

Codecov / codecov/patch

src/jupyterhub_config/custom_docker_spawner.py#L164-L166 

Added lines #L164 - L166 were not covered by tests
f'{mount_base_dir}/{hive_metastore_dir}': {'bind': f'{hive_metastore_dir}', 'mode': access_mode},
f'{mount_base_dir}/{user_home_dir}/{self.user.name}': f'{user_home_dir}/{self.user.name}' # User specific home directory
})
Loading