Skip to content

Commit

Permalink
added port range for restricted vlans
Browse files Browse the repository at this point in the history
  • Loading branch information
pontiggi committed Jun 13, 2018
1 parent c0da1f3 commit 50428e8
Show file tree
Hide file tree
Showing 2 changed files with 28 additions and 6 deletions.
33 changes: 27 additions & 6 deletions batchspawner/batchspawner.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,16 @@
from jupyterhub.spawner import set_user_setuid
import jupyterhub

from port_range import PortRange
def ports_in_range(range1,range2):
range1 = PortRange(range1).bounds
range2 = PortRange(range2).bounds
return range1[0] >= range2[0] and range1[1] <= range2[1]

def random_port_inrange(range1):
import random
return random.randint(PortRange(range1).port_from,PortRange(range1).port_to)

This comment has been minimized.

Copy link
@rkdarst

rkdarst Jul 27, 2018

Using an extra dependency isn't ideal - especially one not in conda or conda-forge.

If you look at jupyterhub.utils.random_port, you see that it also makes sure that the port is free before returning it (by letting OS pick a random free port). To duplicate that here, seems like you may need to try to bind to each port to see if it works or not. But also note issue jupyterhub#58 in batchspawner - actually it doesn't matter since random_port runs on a different host than it runs on. In the grand scheme, these two should be merged somehow.


def format_template(template, *args, **kwargs):
"""Format a template, either using jinja2 or str.format().
Expand Down Expand Up @@ -78,6 +88,9 @@ class BatchSpawnerBase(Spawner):
# override default server ip since batch jobs normally running remotely
ip = Unicode("0.0.0.0", help="Address for singleuser server to listen at").tag(config=True)

# set a port range in case there are network restrictions in the compute network
ports = Unicode("0", help="port range accessible for connections between hub and compute resources").tag(config=True)

This comment has been minimized.

Copy link
@rkdarst

rkdarst Jul 27, 2018

To simplify, could directly be a tuple config option? Then the port_range module isn't needed, and the two bounds are top and bottom. Less flexibility, but is the extra flexibility needed?


exec_prefix = Unicode('sudo -E -u {username}', \
help="Standard executon prefix (e.g. the default sudo -E -u {username})"
).tag(config=True)
Expand Down Expand Up @@ -331,12 +344,20 @@ def poll(self):
@gen.coroutine
def start(self):
"""Start the process"""
if self.user and self.user.server and self.user.server.port:
self.port = self.user.server.port
self.db.commit()
elif (jupyterhub.version_info < (0,7) and not self.user.server.port) or \
(jupyterhub.version_info >= (0,7) and not self.port):
self.port = random_port()

This comment has been minimized.

Copy link
@rkdarst

rkdarst Jul 27, 2018

I think all the new logic could go here only

if not ports_in_range(self.ports,'32768-60999'):
self.log.info('failover to default port range')
if self.user and self.user.server and self.user.server.port:
self.port = self.user.server.port
self.db.commit()

This comment has been minimized.

Copy link
@rkdarst

rkdarst Jul 27, 2018

I wasn't the one that fix this last time, but this relates to restoring old jobs or backwards compatibilty or some such, and probably shouldn't be inside the new conditional.

elif (jupyterhub.version_info < (0,7) and not self.user.server.port) or \
(jupyterhub.version_info >= (0,7) and not self.port):
self.port = random_port()
self.db.commit()
self.log.info(self.port)
else :
self.log.info('using specified port range')
self.port = random_port_inrange(self.ports)
self.log.info(self.port)
self.db.commit()
job = yield self.submit_batch_script()

Expand Down
1 change: 1 addition & 0 deletions requirements.txt
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
jinja2
jupyterhub>=0.5
port_range

0 comments on commit 50428e8

Please sign in to comment.