From 7800c1af02548ee883123e800940361b1d347cdd Mon Sep 17 00:00:00 2001 From: Stefano Borini Date: Tue, 27 Sep 2016 17:14:48 +0100 Subject: [PATCH 1/2] Change of strategy with workspace. Workspace is now created with a defined username and preserved. --- remoteappmanager/spawners.py | 63 +++++++++++++++---------- remoteappmanager/tests/test_spawners.py | 46 +++++++++++------- 2 files changed, 65 insertions(+), 44 deletions(-) diff --git a/remoteappmanager/spawners.py b/remoteappmanager/spawners.py index 167607cb1..82a97b31f 100644 --- a/remoteappmanager/spawners.py +++ b/remoteappmanager/spawners.py @@ -1,6 +1,6 @@ import os -import shutil -import tempfile +import escapism +import string from traitlets import Any, Unicode from tornado import gen @@ -153,8 +153,15 @@ def start(self): # Create the temporary directory as the user's workspace if self.workspace_dir and not self._virtual_workspace: try: - self._virtual_workspace = tempfile.mkdtemp( - dir=self.workspace_dir) + workspace = _user_workspace(self.workspace_dir, self.user.name) + + if not (os.path.isdir(workspace) and + os.access(workspace, os.R_OK | os.W_OK | os.X_OK)): + # Let it fail for strange conditions such as unable to + # write or file already there. + os.mkdir(workspace, 0o755) + + self._virtual_workspace = workspace except Exception as exception: # A whole lot of reasons why temporary directory cannot # be created. e.g. workspace_dir does not exist @@ -178,7 +185,6 @@ def start(self): try: return (yield super().start()) except Exception: - self._clean_up_workspace_dir() raise @gen.coroutine @@ -188,29 +194,8 @@ def stop(self, now=False): If virtual user has a temporary home directory, clean up the directory. """ - self._clean_up_workspace_dir() yield super().stop(now=now) - def _clean_up_workspace_dir(self): - """ Clean up the virtual user's temporary directory, if exists - """ - if not self._virtual_workspace: - return - - # Clean up the directory - # Make sure the temporary directory is not /, ./ or ../ - if os.path.normpath(self._virtual_workspace) in ('/', '.', '..'): - self.log.warning("Virtual workspace is '%s'. Seriously? " - "Not removing.", self._virtual_workspace) - else: - try: - shutil.rmtree(self._virtual_workspace) - except Exception as exception: - self.log.error("Failed to remove %s, error %s", - self._virtual_workspace, str(exception)) - else: - self.log.info('Removed %s', self._virtual_workspace) - def _docker_envvars(): """Returns a dictionary containing the docker environment variables, if @@ -223,3 +208,29 @@ def _docker_envvars(): if envvar in os.environ} return env + + +# Used by escape +_ESCAPE_SAFE_CHARS = set(string.ascii_letters + string.digits + '-.') +_ESCAPE_CHAR = '_' + + +# Note: copied from container_manager.py, but we want to keep the +# spawners module completely separated from the remoteappmanager. +def escape(s): + """Trivial escaping wrapper for well established stuff. + Works for containers, file names. Note that it is not destructive, + so it won't generate collisions.""" + return escapism.escape(s, _ESCAPE_SAFE_CHARS, _ESCAPE_CHAR) + + +def _user_workspace(base_dir, user_name): + """Returns the appropriate user workspace for the given username. + Raises ValueError if the user_name is only spaces after basenaming. + """ + + name = os.path.basename(user_name).strip() + if len(name) == 0: + raise ValueError("User name is invalid") + + return os.path.join(base_dir, escape(name)) diff --git a/remoteappmanager/tests/test_spawners.py b/remoteappmanager/tests/test_spawners.py index 7288d905d..a36040480 100644 --- a/remoteappmanager/tests/test_spawners.py +++ b/remoteappmanager/tests/test_spawners.py @@ -171,12 +171,38 @@ def test_spawner_with_workspace_dir(self): self.assertIn(os.path.basename(virtual_directory), os.listdir(self.tempdir)) - # The temporary directory should be removed upon stop - self.assertFalse(os.listdir(self.tempdir)) + self.assertIn(os.path.basename(virtual_directory), + os.listdir(self.tempdir)) status = self.io_loop.run_sync(self.spawner.poll) self.assertEqual(status, 1) + def test_spawner_with_workspace_dir_already_existent(self): + self.spawner.workspace_dir = self.tempdir + os.mkdir(os.path.join(self.tempdir, username())) + + with spawner_start_and_stop(self.io_loop, self.spawner): + status = self.io_loop.run_sync(self.spawner.poll) + self.assertIsNone(status) + + # There should be a temporary directory created + # and it should be assigned to _virtual_workspace + virtual_directory = self.spawner._virtual_workspace + self.assertIn(os.path.basename(virtual_directory), + os.listdir(self.tempdir)) + + self.assertIn(os.path.basename(virtual_directory), + os.listdir(self.tempdir)) + + def test_spawner_with_workspace_dir_as_file(self): + self.spawner.workspace_dir = self.tempdir + + with open(os.path.join(self.tempdir, username()), 'w'): + pass + + with spawner_start_and_stop(self.io_loop, self.spawner): + self.assertIsNone(self.spawner.get_env().get('HOME')) + def test_env_has_proxy_api_token(self): env = self.spawner.get_env() self.assertIn("PROXY_API_TOKEN", env) @@ -213,22 +239,6 @@ def test_state_if_workspace_not_defined(self): state = self.spawner.get_state() self.assertNotIn('virtual_workspace', state) - def test_clean_up_temporary_dir_if_start_fails(self): - self.spawner.workspace_dir = self.tempdir - - # mock LocalProcessSpawner.start to fail - def start_fail(instance): - raise Exception - - with mock.patch('jupyterhub.spawner.LocalProcessSpawner.start', - start_fail), \ - self.assertRaises(Exception), \ - spawner_start_and_stop(self.io_loop, self.spawner): - pass - - # The temporary directory should be cleaned up - self.assertFalse(os.listdir(self.tempdir)) - def test_start_if_workspace_path_not_exists(self): self.spawner.workspace_dir = '/no_way/this_exists' From 2fbadb913bf5884522fc9015691487aae5d850e2 Mon Sep 17 00:00:00 2001 From: Stefano Borini Date: Tue, 27 Sep 2016 23:15:15 +0100 Subject: [PATCH 2/2] Changed creation strategy --- remoteappmanager/spawners.py | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/remoteappmanager/spawners.py b/remoteappmanager/spawners.py index 82a97b31f..5e12e52ba 100644 --- a/remoteappmanager/spawners.py +++ b/remoteappmanager/spawners.py @@ -154,13 +154,7 @@ def start(self): if self.workspace_dir and not self._virtual_workspace: try: workspace = _user_workspace(self.workspace_dir, self.user.name) - - if not (os.path.isdir(workspace) and - os.access(workspace, os.R_OK | os.W_OK | os.X_OK)): - # Let it fail for strange conditions such as unable to - # write or file already there. - os.mkdir(workspace, 0o755) - + os.makedirs(workspace, 0o755, exist_ok=True) self._virtual_workspace = workspace except Exception as exception: # A whole lot of reasons why temporary directory cannot