Rework req_keepvars handling:

- Rework req_keepvars.  Old meaning of req_keepvars didn't make sense,
  because it overrode mandatory variables that JupyterHub sets.
  Change to these two variables:
  - req_keepvars_default: Automatically set by JupyterHub defaults,
    should not be overridden.  Old req_keepvars becomes this.  Should
    only be changed if you know what you are doing.
  - req_keepvars: Adds to the defaults in req_keepvars_default.  This
    is what cluster admins usually want to set.

Author: rkdarst

CHANGELOG.md

@@ -6,7 +6,6 @@ Added (user)
 
 * Add Jinja2 templating as an option for all scripts and commands.  If '{{' or `{%` is used anywhere in the string, it is used as a jinja2 template.
 * Add new option exec_prefix, which defaults to `sudo -E -u {username}`.  This replaces explicit `sudo` in every batch command - changes in local commands may be needed.
-* New option: `req_keepvars_extra`, which allows keeping extra variables in addition to what is defined by JupyterHub itself (addition of variables to keep instead of replacement).  #99
 * Add `req_prologue` and `req_epilogue` options to scripts which are inserted before/after the main jupyterhub-singleuser command, which allow for generic setup/cleanup without overriding the entire script.  #96
 * SlurmSpawner: add the `req_reservation` option.  #91
 * Add basic support for JupyterHub progress updates, but this is not used much yet.  #86
@@ -25,6 +24,7 @@ Changed
   - Add a new option `batchspawner_singleuser_cmd` which is used as a wrapper in the single-user servers, which conveys the remote port back to JupyterHub.  This is now an integral part of the spawn process.
   - If you have installed with `pip install -e`, you will have to re-install so that the new script `batchspawner-singleuser` is added to `$PATH`.
 * Update minimum requirements to JupyterHub 0.9 and Python 3.5.  #143
++* Change meaning of `req_keepvars`.  Previously, setting this would override everything that JupyterHub requires to be set (so you'd have to make sure you set it back).  Now, `req_keepvars` only *adds* to the JupyterHub defaults, and `req_keepvars_default` gets set to the JupyterHub upstream and can be used to completly override the JupyterHub defaults. #99, #??
 * Update Slurm batch script.  Now, the single-user notebook is run in a job step, with a wrapper of `srun`.  This may need to be removed using `req_srun=''` if you don't want environment variables limited.
 * Pass the environment dictionary to the queue and cancel commands as well.  This is mostly user environment, but may be useful to these commands as well in some cases. #108, #111  If these environment variables were used for authentication as an admin, be aware that there are pre-existing security issues because they may be passed to the user via the batch submit command, see #82.
 

batchspawner/batchspawner.py

@@ -143,15 +143,30 @@ def _req_username_default(self):
     def _req_homedir_default(self):
         return pwd.getpwnam(self.user.name).pw_dir
 
-    req_keepvars = Unicode()
-    @default('req_keepvars')
-    def _req_keepvars_default(self):
+    req_keepvars_default = Unicode(
+        help="All environment variables to pass to the spawner.  This is set "
+             "to a default by JupyterHub, and if you change this list "
+             "the previous ones are _not_ included and your submissions will "
+             "break unless you re-add necessary variables.  Consider "
+             "req_keepvars for most use cases."
+)
+    @default('req_keepvars_all')
+    def _req_keepvars_all_default(self):
         return ','.join(self.get_env().keys())
 
-    req_keepvars_extra = Unicode(
+    req_keepvars = Unicode(
         help="Extra environment variables which should be configured, "
               "added to the defaults in keepvars, "
-              "comma separated list.")
+              "comma separated list.").tag(config=True)
+    admin_environment = Unicode(
+        help="Comma-separated list of environment variables to be passed to "
+             "the batch submit/cancel commands, but _not_ to the batch script "
+             "via --export.  This could be used, for example, to authenticate "
+             "the submit command as an admin user so that it can submit jobs "
+             "as another user.  These are _not_ included in an "
+             "--export={keepvars} type line in the batch script, but you "
+             "should check that your batch system actually does the right "
+             "thing here.").tag(config=True)
 
     batch_script = Unicode('',
         help="Template for job submission script. Traits on this class named like req_xyz "
@@ -177,8 +192,13 @@ def get_req_subvars(self):
         subvars = {}
         for t in reqlist:
             subvars[t[4:]] = getattr(self, t)
-        if subvars.get('keepvars_extra'):
-            subvars['keepvars'] += ',' + subvars['keepvars_extra']
+        # 'keepvars' is special: 'keepvars' goes through as the
+        # variable, but 'keepvars_default' is prepended to it.
+        # 'keepvars_default' is JH-required stuff so you have to try
+        # extra hard to override it.
+        if subvars.get('keepvars'):
+            subvars['keepvars_default'] += ',' + subvars['keepvars']
+        subvars['keepvars'] = subvars['keepvars_default']
         return subvars
 
     batch_submit_cmd = Unicode('',

batchspawner/tests/test_spawners.py

@@ -491,7 +491,7 @@ def test_lfs(db, io_loop):
 def test_keepvars(db, io_loop):
     # req_keepvars
     spawner_kwargs = {
-        'req_keepvars': 'ABCDE',
+        'req_keepvars_default': 'ABCDE',
         }
     batch_script_re_list = [
         re.compile(r'--export=ABCDE', re.X|re.M),
@@ -500,10 +500,32 @@ def test_keepvars(db, io_loop):
                               spawner_kwargs=spawner_kwargs,
                               batch_script_re_list=batch_script_re_list)
 
-    # req_keepvars AND req_keepvars together
+    # req_keepvars
     spawner_kwargs = {
         'req_keepvars': 'ABCDE',
-        'req_keepvars_extra': 'XYZ',
+        }
+    batch_script_re_list = [
+        re.compile(r'--export=.*ABCDE', re.X|re.M),
+        ]
+    run_typical_slurm_spawner(db, io_loop,
+                              spawner_kwargs=spawner_kwargs,
+                              batch_script_re_list=batch_script_re_list)
+
+    # req_keepvars
+    spawner_kwargs = {
+        'admin_environment': 'ABCDE',
+        }
+    batch_script_re_list = [
+        re.compile(r'^((?!ABCDE).)*$', re.X|re.S),
+        ]
+    run_typical_slurm_spawner(db, io_loop,
+                              spawner_kwargs=spawner_kwargs,
+                              batch_script_re_list=batch_script_re_list)
+
+    # req_keepvars AND req_keepvars together
+    spawner_kwargs = {
+        'req_keepvars_default': 'ABCDE',
+        'req_keepvars': 'XYZ',
         }
     batch_script_re_list = [
         re.compile(r'--export=ABCDE,XYZ', re.X|re.M),