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

Added support for named-servers #78

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,5 +23,6 @@
install_requires=[
"jupyterhub>=0.9",
"tornado>=5.0",
"escapism"
],
)
47 changes: 40 additions & 7 deletions systemdspawner/systemdspawner.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@
import subprocess
from traitlets import Bool, Unicode, List, Dict
import asyncio
import string

import escapism
from systemdspawner import systemd

from jupyterhub.spawner import Spawner
Expand Down Expand Up @@ -53,11 +55,11 @@ class SystemdSpawner(Spawner):
).tag(config=True)

unit_name_template = Unicode(
'jupyter-{USERNAME}-singleuser',
'jupyter-{USERNAME}-singleuser-{SERVERNAME}',
help="""
Template to use to make the systemd service names.

{USERNAME} and {USERID} are expanded}
{USERNAME}, {SERVERNAME}, and {USERID} are expanded}
Copy link
Member

Choose a reason for hiding this comment

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

{USERID} has been replaced by {USERNAME} for the unit name, so it can be removed as it's not longer used.

"""
).tag(config=True)

Expand Down Expand Up @@ -147,25 +149,55 @@ class SystemdSpawner(Spawner):
"""
).tag(config=True)

unit_trailing_character = Unicode(
'-',
help="""
When using a unit that ends in a SERVERNAME strip this character from the end of the parsed string.
"""
).tag(config=True)

escape_char = Unicode(
':',
help="""
The character to change invalid safe characters to.
"""
).tag(config=True)
Comment on lines +152 to +164
Copy link
Member

Choose a reason for hiding this comment

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

Making these configurable might open up the room for possible miss configurations. Let's just add this info in the help description of unit_name_template.


# Characters that are safe for systemd units.
safe_chars = set(string.ascii_lowercase + string.digits + string.ascii_uppercase + ':-_.\\')
Copy link
Member

Choose a reason for hiding this comment

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

The escape character cannot be in the list of safe chars. Not sure if this causes an error or a warning, but it's best to remove it.

max_unit_length = 248

def __init__(self, *args, **kwargs):
super().__init__(*args, **kwargs)
# All traitlets configurables are configured by now
self.unit_name = self._expand_user_vars(self.unit_name_template)

self.log.debug('user:%s Initialized spawner with unit %s', self.user.name, self.unit_name)

def _expand_user_vars(self, string):
def _escape_variable(self, in_string):
"""
Escape variables for systemd unit naming
"""
if len(in_string) > self.max_unit_length:
self.log.warning(f'String is longer than {self.max_unit_length}')
# Slice the string to the max unit length
return escapism.escape(in_string[:self.max_unit_length], safe=self.safe_chars, escape_char=self.escape_char)
Comment on lines +181 to +184
Copy link
Member

Choose a reason for hiding this comment

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

I was thinking that rather than slice the unit name ourselves if it exceeds max length, we should instead throw an error if this happens.

The reason why I think this would be better is because if we slice it, the unit name will not respect the template anymore, and one might end up with duplicates.


def _expand_user_vars(self, in_string):
"""
Expand user related variables in a given string

Currently expands:
{USERNAME} -> Name of the user
{USERID} -> UserID
{SERVERNAME} -> Name of the server (self.name)
"""
return string.format(
# Strip the trailing - if we don't have a name.
return self._escape_variable(in_string.format(
USERNAME=self.user.name,
USERID=self.user.id
)
USERID=self.user.id,
Copy link
Member

Choose a reason for hiding this comment

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

USERID can be removed from here also, as it's not used anymore for the unit name.

SERVERNAME=self.name
)).rstrip(self.unit_trailing_character)

def get_state(self):
"""
Expand Down Expand Up @@ -205,7 +237,8 @@ async def start(self):
# from earlier. Regardless, we kill it and start ours in its place.
# FIXME: Carefully look at this when doing a security sweep.
if await systemd.service_running(self.unit_name):
self.log.info('user:%s Unit %s already exists but not known to JupyterHub. Killing', self.user.name, self.unit_name)
self.log.info('user:%s Unit %s already exists but not known to JupyterHub. Killing', self.user.name,
self.unit_name)
await systemd.stop_service(self.unit_name)
if await systemd.service_running(self.unit_name):
self.log.error('user:%s Could not stop already existing unit %s', self.user.name, self.unit_name)
Expand Down